Feature/blog editor upgrade #22
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughUpdated namespaces to Neuron\Data\Settings; added Editor.js content storage and renderer; Cloudinary media uploader (with SSRF checks), validator, DTO and admin endpoints; many repositories moved to ORM; added events, Auth+CSRF filter, cms:upgrade CLI, new migrations, tests, CI and docs. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant CMS
participant Cloudinary
participant DB
User->>Browser: Compose post in EditorJS (may embed image)
Browser->>CMS: POST /admin/upload/image (multipart)
CMS->>CMS: MediaValidator.validate(file)
alt invalid
CMS-->>Browser: 400 EditorJS error JSON
else valid
CMS->>Cloudinary: UploadApi.upload(file)
Cloudinary-->>CMS: upload response (url, public_id, meta)
CMS-->>Browser: 200 EditorJS success JSON
end
Browser->>CMS: POST /admin/posts (content_raw JSON + metadata)
CMS->>CMS: Creator.setContent(content_raw) → extractPlainText → setBody
CMS->>DB: INSERT/UPDATE post (content_raw, body, ...)
DB-->>CMS: success + id
CMS-->>Browser: Redirect / success
Browser->>CMS: GET /blog/{slug}
CMS->>DB: SELECT post (content_raw)
DB-->>CMS: content_raw JSON
CMS->>CMS: EditorJsRenderer.render(content_raw)
CMS->>CMS: ShortcodeParser.parse(rendered HTML)
CMS-->>Browser: 200 rendered page
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Focus areas:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Cms/Services/Auth/Authentication.php (1)
226-249: Fix infinite recursion on stale sessions inuser()/logout()In
user():if( !$user ) { // Clear stale session if user no longer exists $this->logout(); }If the session contains a
user_idfor a user that has been deleted, this callslogout(), which in turn calls$this->check()and$this->user(), triggering the same path again and causing infinite recursion (now with additional event logic on each loop).Instead of delegating to
logout(), clear the stale session inline:- $user = $this->_userRepository->findById( $userId ); - - if( !$user ) - { - // Clear stale session if user no longer exists - $this->logout(); - } - - return $user; + $user = $this->_userRepository->findById( $userId ); + + if( !$user ) + { + // Clear stale session and remember-me cookie without recursion + $this->_sessionManager->destroy(); + + if( isset( $_COOKIE['remember_token'] ) ) + { + setcookie( 'remember_token', '', time() - 3600, '/', '', true, true ); + } + + return null; + } + + return $user;This preserves the intended behavior (dropping an invalid session and remember-me cookie) without risking a stack overflow or runaway event emissions when the underlying user record is missing.
src/Cms/Controllers/Blog.php (1)
87-93: "Not found" post will have empty content for rendering.When a post is not found, a dummy Post object is created with
setBody()but the new rendering pipeline usesgetContent()which will return the default empty EditorJS structure{"blocks":[]}. This means the "not found" message won't be displayed.Update the "not found" post to use the new content structure:
if( !$post ) { $post = new Post(); $post->setTitle( 'Article Not Found' ); - $post->setBody( 'The requested article does not exist.' ); + $post->setContent( json_encode([ + 'blocks' => [ + [ + 'type' => 'paragraph', + 'data' => ['text' => 'The requested article does not exist.'] + ] + ] + ]) ); $post->setSlug( $slug ); }
🧹 Nitpick comments (16)
resources/views/http_codes/500.php (2)
8-20: Inline styles are appropriate for error pages.Using inline styles ensures the page displays correctly even if external stylesheets fail to load, which is a good practice for error pages.
Optional: Consider adding responsive design for smaller screens:
.centered { text-align: center; - margin-top: 100px; /* Adjust this value as needed */ + margin-top: 10vh; + padding: 0 15px; } .centered-text { text-align: center; margin-top: 20px; font-size: 16px; color: #666; + padding: 0 15px; }
1-6: Consider adding accessibility attributes and user escape hatches.The error page would benefit from:
- ARIA landmark roles for screen readers
- A link back to the home page or contact information
-<div class="row"> - <div class="col"> +<div class="row" role="main"> + <div class="col"> <h1 class="centered">Something went wrong on our end.</h1> <p class="centered-text">The server encountered an unexpected error. Please try again later.</p> + <p class="centered-text"><a href="/" style="color: #0066cc;">Return to Home</a></p> </div> </div>src/Cms/Repositories/DatabaseCategoryRepository.php (1)
211-217: GROUP BY may break on PostgreSQL
allWithPostCount()selectsc.*but groups only byc.id; PostgreSQL’s stricter GROUP BY rules will reject this query. If you intend this repository to work against PostgreSQL (per the class doc), you’ll need to either:
- Group by all selected
categoriescolumns, or- Select only grouped/aggregated columns.
Please confirm whether PostgreSQL is a supported target in your deployments; if so, consider adjusting the SELECT/GROUP BY to be portable.
composer.json (1)
15-27: Dependency updates align with the Settings refactor; confirm version rangesThe move to
neuron-php/mvc: "0.9.*"and addition ofneuron-php/data: "0.9.*"andneuron-php/scaffolding: "0.8.*"fit the new Settings namespace and tooling story.One nuance:
"0.9.*"can allow earlier 0.9.x releases than^0.9.5would have; in practice Composer will usually pick the latest matching version, but if you depend on features/bugfixes introduced in later 0.9.x, you may prefer a caret or a higher minimum.Please make sure
composer.lockis updated and CI (including migrations/tests that touch Settings) passes with these new constraints. If you want to avoid accidentally downgrading, consider tightening the constraints (e.g.,^0.9.5/^0.9.0) before merging.src/Cms/Repositories/DatabasePostRepository.php (1)
84-103: Verify handling of the newcontent_rawcolumn in postsThe new migration adds a non‑nullable
content_rawcolumn intended for Editor.js JSON, but this repository currently:
- Inserts/updates only
body/excerpt(nocontent_rawin the column/value lists), and- Does not include
content_rawin the$dataarray passed toPost::fromArray().Because the DB default will populate
content_raw, this won’t break inserts, but any Editor.js JSON stored on the entity may not be persisted or hydrated unless other layers handle it.Please double‑check how the
Postmodel exposes raw Editor.js content (e.g.,getContentRaw()/setContentRaw()or similar), and consider extending:
- the INSERT/UPDATE statements, and
mapRowToPost()’s$dataarray
to read/writecontent_rawif that field is meant to be first‑class in the domain.Also applies to: 141-168, 440-455
src/Cms/Services/Auth/PasswordResetter.php (1)
106-110: LGTM! Event emissions enhance observability.The addition of
PasswordResetRequestedEventandPasswordResetCompletedEventprovides valuable audit trails for password reset workflows. The fallback to'unknown'for$_SERVER['REMOTE_ADDR']appropriately handles CLI and testing contexts.Consider importing the event classes at the top of the file rather than using fully qualified names for improved readability:
+use Neuron\Cms\Events\PasswordResetRequestedEvent; +use Neuron\Cms\Events\PasswordResetCompletedEvent; +use Neuron\Application\CrossCutting\Event;Then use:
- \Neuron\Application\CrossCutting\Event::emit( new \Neuron\Cms\Events\PasswordResetRequestedEvent( + Event::emit( new PasswordResetRequestedEvent(Also applies to: 175-179
resources/views/admin/posts/edit.php (2)
24-31: Harden handling of existing EditorJS content JSON before initializationCurrent parsing only falls back on JSON syntax errors; valid-but-unexpected JSON (e.g.
"null"or an object withoutblocks) will flow intodataand may confuse EditorJS.You can defensively ensure a valid
blocksarray after parsing:-let existingContent; -try { - existingContent = JSON.parse(existingContentJson); -} catch (error) { - console.error('Failed to parse existing content:', error); - existingContent = { blocks: [] }; -} +let existingContent; +try { + existingContent = JSON.parse(existingContentJson); + + // Ensure we always pass a valid Editor.js data structure + if( !existingContent || !Array.isArray( existingContent.blocks ) ) + { + existingContent = { blocks: [] }; + } +} catch (error) { + console.error('Failed to parse existing content:', error); + existingContent = { blocks: [] }; +}This keeps the editor resilient to legacy or malformed
content_rawvalues while preserving your existing fallback behavior.Also applies to: 93-103, 104-109
82-90: Consider pinning EditorJS CDN versions and reducingsave()frequencyThe EditorJS integration and submit flow look solid. Two optional robustness points:
- The
@latestCDN URLs can introduce breaking changes when a new major EditorJS (or tool) version ships. Consider pinning to a specific major/minor version (e.g.@2.xor an exact version) and upgrading deliberately.onChangecurrently callseditor.save()on every change, which is relatively heavy. If this becomes a performance issue for long posts, you could debounce/throttle the save into#content-jsonor rely solely on the submit handler to serialize content.Both are optional, but would make the editor behavior more predictable in production.
Also applies to: 145-149, 151-163
tests/Cms/Services/Post/CreatorTest.php (1)
45-69: Centralize EditorJS content JSON in a helper/fixture for test readabilityThe tests correctly assert both
getContentRaw()and the derivedgetBody(). The same EditorJS JSON pattern ({"blocks":[{"type":"paragraph","data":{"text":"Body"}}]}and variants) is repeated across multiple tests, though.To keep things DRY and easier to tweak later, consider extracting small helpers/constants, e.g.:
private function makeEditorJsContent(string $text = 'Body'): string { return json_encode([ 'blocks' => [ [ 'type' => 'paragraph', 'data' => ['text' => $text], ], ], ]); }Then use
makeEditorJsContent('Test body content')/makeEditorJsContent()throughout the tests instead of hardcoding JSON strings.Also applies to: 90-95, 118-123, 148-153, 177-182, 218-223, 263-273, 297-305
src/Cms/Maintenance/MaintenanceManager.php (1)
61-82: Emit maintenance events without risking core enable/disable operationsThe new enabled/disabled events are wired in cleanly, and the optional
$disabledByargument keeps the public API backward compatible.One robustness refinement to consider: if any event listener hooked into
MaintenanceModeEnabledEvent/MaintenanceModeDisabledEventthrows, callers ofenable()/disable()will see an exception even though the maintenance file has already been written or deleted. To keep state changes reliable, you might wrap the emits:- $result = $this->writeMaintenanceFile( $data ); - - // Emit maintenance mode enabled event - if( $result ) - { - \Neuron\Application\CrossCutting\Event::emit( new \Neuron\Cms\Events\MaintenanceModeEnabledEvent( - $data['enabled_by'], - $message - ) ); - } + $result = $this->writeMaintenanceFile( $data ); + + // Emit maintenance mode enabled event (non-fatal) + if( $result ) + { + try + { + \Neuron\Application\CrossCutting\Event::emit( + new \Neuron\Cms\Events\MaintenanceModeEnabledEvent( + $data['enabled_by'], + $message + ) + ); + } + catch( \Throwable $e ) + { + // Optional: log but do not affect enable() result + } + }and similarly in
disable(). That keeps maintenance mode toggling deterministic even if downstream listeners misbehave.Also applies to: 87-107
src/Cms/Services/Auth/Authentication.php (1)
41-59: Event emissions for login attempts look good; consider small refactorsThe new
UserLoginFailedEvent,UserLoginEvent, andUserLogoutEventhooks are well-placed and give you good observability over auth flows. A couple of low-friction refinements:
- The four failure branches emit
UserLoginFailedEventwith only thereasondiffering. You could extract a small private helper likeemitLoginFailed(string $username, string $reason): voidto reduce duplication and keep changes localized if you later enrich the payload.- In
login(), you callmicrotime( true )both when settinglogin_timein the session and when emittingUserLoginEvent. Reusing a single timestamp would keep those values perfectly aligned:- // Store user ID in session - $this->_sessionManager->set( 'user_id', $user->getId() ); - $this->_sessionManager->set( 'user_role', $user->getRole() ); - $this->_sessionManager->set( 'login_time', microtime( true ) ); + // Store user ID in session + $this->_sessionManager->set( 'user_id', $user->getId() ); + $this->_sessionManager->set( 'user_role', $user->getRole() ); + $loginTime = microtime( true ); + $this->_sessionManager->set( 'login_time', $loginTime ); @@ - // Emit user login event - \Neuron\Application\CrossCutting\Event::emit( new \Neuron\Cms\Events\UserLoginEvent( - $user, - $_SERVER['REMOTE_ADDR'] ?? 'unknown', - microtime( true ) - ) ); + // Emit user login event + \Neuron\Application\CrossCutting\Event::emit( new \Neuron\Cms\Events\UserLoginEvent( + $user, + $_SERVER['REMOTE_ADDR'] ?? 'unknown', + $loginTime + ) );Overall the behavior is sound; these are just to keep things DRY and metrics-friendly.
Also applies to: 61-85, 87-110, 134-156, 161-201
tests/Cms/Services/Post/UpdaterTest.php (1)
39-42: Mirror CreatorTest by centralizing EditorJS JSON for updatesThese tests correctly validate that
Updater::update()preserves both the raw EditorJS JSON (getContentRaw()) and the derived body text (getBody()), and that other fields (slug, status, categories, tags, etc.) behave as expected.As in
CreatorTest, you might want to centralize the repeated JSON snippets into a small helper (on this class or a shared trait) so any future changes to your EditorJS block shape only need to be made in one place.Also applies to: 54-70, 97-99, 127-128, 171-172, 218-219, 254-255, 285-286, 321-322
resources/views/admin/posts/create.php (1)
131-142: Consider potential issues with programmatic form submission.The submit handler prevents the default form submission, saves editor content, then calls
e.target.submit(). This bypasses normal HTML5 validation and could cause issues if there are other submit handlers or validation logic.Consider using FormData to submit the form programmatically while maintaining validation:
-document.getElementById('post-form').addEventListener('submit', async (e) => { - e.preventDefault(); - - try { - const savedData = await editor.save(); - document.getElementById('content-json').value = JSON.stringify(savedData); - e.target.submit(); - } catch (error) { - console.error('Error saving editor content:', error); - alert('Error preparing content. Please try again.'); - } -}); +document.getElementById('post-form').addEventListener('submit', async (e) => { + // Don't prevent default initially - let HTML5 validation run first + if (!e.target.checkValidity()) { + return; // Let browser handle validation + } + + e.preventDefault(); + + try { + const savedData = await editor.save(); + document.getElementById('content-json').value = JSON.stringify(savedData); + e.target.submit(); + } catch (error) { + console.error('Error saving editor content:', error); + alert('Error preparing content. Please try again.'); + } +});src/Cms/Controllers/Blog.php (1)
106-108: Add error handling for content rendering.The renderer should handle malformed JSON or rendering errors gracefully to prevent the page from breaking when post content is invalid.
Wrap the rendering in a try-catch block:
- // Render content from Editor.js JSON - $content = $post->getContent(); - $renderedContent = $this->_renderer->render( $content ); + // Render content from Editor.js JSON + $content = $post->getContent(); + try { + $renderedContent = $this->_renderer->render( $content ); + } catch (\Exception $e) { + error_log("Error rendering post content for slug '{$slug}': " . $e->getMessage()); + $renderedContent = '<p>Content temporarily unavailable.</p>'; + }src/Cms/Models/Post.php (2)
125-128: Add error handling for malformed JSON in getContent().If
_contentRawcontains invalid JSON,json_decode()will returnnulland the??operator will correctly return the default, but this happens silently. Consider logging the error for debugging.public function getContent(): array { - return json_decode( $this->_contentRaw, true ) ?? ['blocks' => []]; + $decoded = json_decode( $this->_contentRaw, true ); + if ($decoded === null && json_last_error() !== JSON_ERROR_NONE) { + error_log("Invalid JSON in Post content_raw (ID: {$this->_id}): " . json_last_error_msg()); + } + return $decoded ?? ['blocks' => []]; }
607-646: Consider XSS risks in extractPlainText() for 'raw' block type.Line 633 includes raw HTML from
rawblocks without sanitization when extracting plain text. Whilestrip_tags()is applied at line 640, malicious HTML could still be present in the intermediate processing.Since this is meant to be plain text extraction, consider whether
rawblocks should be included at all, or strip tags immediately:$blockText = match( $block['type'] ) { 'paragraph', 'header' => $block['data']['text'] ?? '', 'list' => isset( $block['data']['items'] ) && is_array( $block['data']['items'] ) ? implode( "\n", $block['data']['items'] ) : '', 'quote' => $block['data']['text'] ?? '', 'code' => $block['data']['code'] ?? '', - 'raw' => $block['data']['html'] ?? '', + 'raw' => strip_tags($block['data']['html'] ?? ''), // Strip tags immediately for raw HTML default => '' }; if( $blockText !== '' ) { // Strip HTML tags from text $blockText = strip_tags( $blockText ); $text[] = trim( $blockText ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (61)
composer.json(2 hunks)resources/app/Initializers/AuthInitializer.php(1 hunks)resources/app/Initializers/MaintenanceInitializer.php(1 hunks)resources/app/Initializers/PasswordResetInitializer.php(1 hunks)resources/app/Initializers/RegistrationInitializer.php(1 hunks)resources/database/migrate/20251119224525_add_content_raw_to_posts.php(1 hunks)resources/views/admin/posts/create.php(3 hunks)resources/views/admin/posts/edit.php(3 hunks)resources/views/blog/show.php(2 hunks)resources/views/http_codes/500.php(1 hunks)src/Bootstrap.php(1 hunks)src/Cms/Cli/Commands/Generate/EmailCommand.php(0 hunks)src/Cms/Cli/Commands/Generate/stubs/email.stub(0 hunks)src/Cms/Cli/Commands/Install/InstallCommand.php(1 hunks)src/Cms/Cli/Commands/Maintenance/EnableCommand.php(1 hunks)src/Cms/Cli/Commands/Queue/InstallCommand.php(0 hunks)src/Cms/Cli/Provider.php(0 hunks)src/Cms/Controllers/Admin/Categories.php(1 hunks)src/Cms/Controllers/Admin/Profile.php(1 hunks)src/Cms/Controllers/Admin/Users.php(1 hunks)src/Cms/Controllers/Blog.php(4 hunks)src/Cms/Controllers/Content.php(4 hunks)src/Cms/Database/ConnectionFactory.php(1 hunks)src/Cms/Email/helpers.php(1 hunks)src/Cms/Events/EmailVerifiedEvent.php(1 hunks)src/Cms/Events/MaintenanceModeDisabledEvent.php(1 hunks)src/Cms/Events/MaintenanceModeEnabledEvent.php(1 hunks)src/Cms/Events/PasswordResetCompletedEvent.php(1 hunks)src/Cms/Events/PasswordResetRequestedEvent.php(1 hunks)src/Cms/Events/UserLoginEvent.php(1 hunks)src/Cms/Events/UserLoginFailedEvent.php(1 hunks)src/Cms/Events/UserLogoutEvent.php(1 hunks)src/Cms/Maintenance/MaintenanceConfig.php(1 hunks)src/Cms/Maintenance/MaintenanceManager.php(1 hunks)src/Cms/Models/Post.php(5 hunks)src/Cms/Repositories/DatabaseCategoryRepository.php(1 hunks)src/Cms/Repositories/DatabaseEmailVerificationTokenRepository.php(1 hunks)src/Cms/Repositories/DatabasePageRepository.php(1 hunks)src/Cms/Repositories/DatabasePasswordResetTokenRepository.php(1 hunks)src/Cms/Repositories/DatabasePostRepository.php(1 hunks)src/Cms/Repositories/DatabaseTagRepository.php(1 hunks)src/Cms/Repositories/DatabaseUserRepository.php(1 hunks)src/Cms/Services/Auth/Authentication.php(5 hunks)src/Cms/Services/Auth/EmailVerifier.php(2 hunks)src/Cms/Services/Auth/PasswordResetter.php(3 hunks)src/Cms/Services/Email/Sender.php(1 hunks)src/Cms/Services/Member/RegistrationService.php(1 hunks)src/Cms/Services/Post/Creator.php(3 hunks)src/Cms/Services/Post/Updater.php(3 hunks)tests/Cms/BlogControllerTest.php(1 hunks)tests/Cms/Cli/Commands/Install/InstallCommandTest.php(1 hunks)tests/Cms/ContentControllerTest.php(1 hunks)tests/Cms/Maintenance/MaintenanceConfigTest.php(1 hunks)tests/Cms/Repositories/DatabaseEmailVerificationTokenRepositoryTest.php(1 hunks)tests/Cms/Repositories/DatabaseUserRepositoryTest.php(1 hunks)tests/Cms/Services/AuthenticationTest.php(1 hunks)tests/Cms/Services/EmailVerifierTest.php(1 hunks)tests/Cms/Services/PasswordResetterTest.php(1 hunks)tests/Cms/Services/Post/CreatorTest.php(9 hunks)tests/Cms/Services/Post/UpdaterTest.php(10 hunks)tests/Cms/Services/RegistrationServiceTest.php(1 hunks)
💤 Files with no reviewable changes (4)
- src/Cms/Cli/Provider.php
- src/Cms/Cli/Commands/Generate/EmailCommand.php
- src/Cms/Cli/Commands/Queue/InstallCommand.php
- src/Cms/Cli/Commands/Generate/stubs/email.stub
🧰 Additional context used
🧬 Code graph analysis (15)
src/Cms/Events/PasswordResetRequestedEvent.php (2)
src/Cms/Events/EmailVerifiedEvent.php (2)
__construct(29-33)getName(35-38)src/Cms/Events/PasswordResetCompletedEvent.php (2)
__construct(30-35)getName(37-40)
resources/views/admin/posts/create.php (1)
src/Cms/View/helpers.php (1)
route_path(70-79)
src/Cms/Events/MaintenanceModeDisabledEvent.php (2)
src/Cms/Events/EmailVerifiedEvent.php (2)
__construct(29-33)getName(35-38)src/Cms/Events/MaintenanceModeEnabledEvent.php (2)
__construct(30-35)getName(37-40)
src/Cms/Events/UserLoginEvent.php (3)
src/Cms/Events/EmailVerifiedEvent.php (2)
__construct(29-33)getName(35-38)src/Cms/Events/UserLoginFailedEvent.php (2)
__construct(31-38)getName(40-43)src/Cms/Events/UserLogoutEvent.php (2)
__construct(30-35)getName(37-40)
src/Cms/Events/UserLoginFailedEvent.php (3)
src/Cms/Events/EmailVerifiedEvent.php (2)
__construct(29-33)getName(35-38)src/Cms/Events/UserLoginEvent.php (2)
__construct(31-37)getName(39-42)src/Cms/Events/UserLogoutEvent.php (2)
__construct(30-35)getName(37-40)
src/Cms/Maintenance/MaintenanceManager.php (2)
src/Cms/Events/MaintenanceModeEnabledEvent.php (1)
MaintenanceModeEnabledEvent(24-41)src/Cms/Events/MaintenanceModeDisabledEvent.php (1)
MaintenanceModeDisabledEvent(24-39)
tests/Cms/Services/Post/CreatorTest.php (2)
src/Cms/Controllers/Content.php (1)
getTitle(141-144)src/Cms/Models/Post.php (2)
getTitle(74-77)getContentRaw(133-136)
src/Cms/Events/MaintenanceModeEnabledEvent.php (2)
src/Cms/Events/EmailVerifiedEvent.php (2)
__construct(29-33)getName(35-38)src/Cms/Events/MaintenanceModeDisabledEvent.php (2)
__construct(29-33)getName(35-38)
src/Cms/Services/Post/Creator.php (1)
src/Cms/Models/Post.php (1)
setContent(142-147)
src/Cms/Services/Auth/EmailVerifier.php (2)
src/Cms/Events/EmailVerifiedEvent.php (1)
EmailVerifiedEvent(24-39)src/Cms/Services/Auth/Authentication.php (1)
user(226-249)
resources/views/admin/posts/edit.php (2)
src/Cms/View/helpers.php (1)
route_path(70-79)src/Cms/Models/Post.php (2)
getId(57-60)getContentRaw(133-136)
src/Cms/Services/Auth/Authentication.php (6)
src/Cms/Events/UserLoginFailedEvent.php (1)
UserLoginFailedEvent(23-44)src/Cms/Models/User.php (2)
isLockedOut(332-340)isActive(187-190)src/Cms/Auth/SessionManager.php (1)
set(99-103)src/Cms/Events/UserLoginEvent.php (1)
UserLoginEvent(24-43)src/Cms/Controllers/Auth/Login.php (1)
logout(139-143)src/Cms/Events/UserLogoutEvent.php (1)
UserLogoutEvent(24-41)
resources/database/migrate/20251119224525_add_content_raw_to_posts.php (1)
src/Cms/Services/Post/Updater.php (1)
update(48-83)
tests/Cms/Services/Post/UpdaterTest.php (2)
src/Cms/Models/Post.php (5)
setContent(142-147)getTitle(74-77)getContentRaw(133-136)getBody(108-111)getStatus(246-249)src/Cms/Repositories/DatabasePostRepository.php (1)
update(127-187)
src/Cms/Services/Post/Updater.php (1)
src/Cms/Models/Post.php (1)
setContent(142-147)
🔇 Additional comments (54)
src/Cms/Maintenance/MaintenanceConfig.php (1)
5-5: Namespace migration is complete and verified.The import change from
Neuron\Data\SettingtoNeuron\Data\Settingson line 5 is correct. Verification confirms no remaining old namespace references exist in the codebase, and the new namespace is consistently used across 20 files (production code and tests).src/Cms/Cli/Commands/Maintenance/EnableCommand.php (1)
8-8: Yaml settings source namespace update looks correctThe Yaml import now targets
Neuron\Data\Settings\Source\Yaml, which matches how it’s used inloadConfiguration()and aligns with the Settings namespace migration; no further changes needed here as long as the installedneuron-php/dataversion exposes this namespace.src/Cms/Services/Email/Sender.php (1)
7-7: SettingManager import aligned with Settings namespaceSwitching to
Neuron\Data\Settings\SettingManageris consistent with the property/ctor type hints and the rest of the Settings migration; the email config access remains unchanged.src/Cms/Services/Member/RegistrationService.php (1)
9-9: RegistrationService now correctly targets Settings\SettingManagerThe import of
Neuron\Data\Settings\SettingManagermatches the constructor and property types and keeps the member settings lookups intact.src/Cms/Controllers/Admin/Categories.php (1)
11-11: Categories controller import updated to Settings\SettingManagerThe controller now imports
Neuron\Data\Settings\SettingManager, keeping it consistent with the rest of the Settings namespace migration in the project.tests/Cms/Maintenance/MaintenanceConfigTest.php (1)
6-6: Test Yaml source import matches new Settings namespaceUsing
Neuron\Data\Settings\Source\Yamlin this test is consistent with the production MaintenanceConfig usage and the broader Settings namespace move.src/Cms/Repositories/DatabaseEmailVerificationTokenRepository.php (1)
7-7: Repository constructor now depends on Settings\SettingManagerThe repository correctly imports
Neuron\Data\Settings\SettingManagerand passes it toConnectionFactory::createFromSettings(), keeping database configuration behavior unchanged under the new namespace.tests/Cms/Repositories/DatabaseEmailVerificationTokenRepositoryTest.php (1)
8-8: Test mock updated to Settings\SettingManagerThe test now mocks
Neuron\Data\Settings\SettingManager, matching the repository’s constructor signature and ensuring the database config wiring remains valid.tests/Cms/ContentControllerTest.php (1)
6-6: In-memory settings source switched to Settings\Source\MemoryThe test now uses
Neuron\Data\Settings\Source\Memory, which is consistent with the Settings namespace refactor and how the controller reads settings via the Registry.src/Cms/Repositories/DatabaseUserRepository.php (1)
7-7: Repository constructor now aligned withSettingsnamespaceThe repository’s dependency on
SettingManageris correctly switched to theNeuron\Data\Settingsnamespace, keeping the constructor type-hint andConnectionFactory::createFromSettings()usage consistent.Please run your user-related tests (e.g.,
DatabaseUserRepositoryTest, auth flows) to confirm there are no remaining references to the oldNeuron\Data\Setting\SettingManagertype.src/Cms/Repositories/DatabasePageRepository.php (1)
8-8: Page repositorySettingManagerimport migrated cleanlyThe constructor’s
SettingManagerdependency now points atNeuron\Data\Settings\SettingManager, matching the rest of the PR’s namespace migration without altering any repository behavior.Confirm that any code instantiating
DatabasePageRepository(controllers/services) is also using the updatedSettings-namespaceSettingManagerinstance from the container/registry.src/Cms/Repositories/DatabaseTagRepository.php (1)
7-7: Tag repository settings dependency updated to new namespace
DatabaseTagRepositorynow correctly importsNeuron\Data\Settings\SettingManagerwhile keeping the constructor contract and internals the same, so behavior should be unaffected.As part of your test run, ensure any install/seed or tag-related CLI flows still resolve the correct
SettingManagerimplementation.tests/Cms/Cli/Commands/Install/InstallCommandTest.php (1)
8-9: Install command test imports aligned withSettingsnamespaceThe test now imports
Neuron\Data\Settings\SettingManagerandNeuron\Data\Settings\Source\Yaml, matching the production code’s updated namespaces and avoiding mismatch with the newer data/settings package layout.If these imports are no longer needed by the test, they can be safely removed in a later cleanup pass once static analysis (e.g., phpstan/psalm) is green.
src/Cms/Controllers/Admin/Users.php (1)
13-13: Admin Users controller now consistent withSettings-based repositoriesThe controller’s import of
Neuron\Data\Settings\SettingManagermatches the updated constructor dependency ofDatabaseUserRepository, keeping the Registry-provided'Settings'instance aligned with the repository’s expected type.Please verify that the Registry is registering an instance of the new
Neuron\Data\Settings\SettingManagerclass under the'Settings'key in your bootstrap/initializer code.src/Bootstrap.php (1)
4-5: Bootstrap imports updated to new data library namespaces
VersionandYamlnow import fromNeuron\Data\Objects\VersionandNeuron\Data\Settings\Source\Yaml, which matches the reorganized data library structure without changing theboot()logic.After updating composer dependencies, run a full application bootstrap (and any bootstrap-related tests) to confirm there are no unresolved references to the old
Neuron\Data\Object\VersionorNeuron\Data\Setting\Source\Yamlclasses.src/Cms/Controllers/Admin/Profile.php (1)
10-10: Admin Profile controller settings import matches repository expectationsThe profile controller’s import now targets
Neuron\Data\Settings\SettingManager, aligning its dependency graph with the updatedDatabaseUserRepositoryand the broader Settings namespace migration.As with the Users controller, double‑check that the Registry’s
'Settings'entry is an instance of the newSettings\SettingManagerto avoid subtle type mismatches at runtime.src/Cms/Database/ConnectionFactory.php (1)
5-5: Namespace migration verified as completeThe import change to
Neuron\Data\Settings\SettingManageris correct and actively used. Verification confirms:
- No lingering references to the old
Neuron\Data\Settingnamespace exist- The type hint in
createFromSettings()(line 26) properly references the newSettingManagerclass- The method implementation correctly delegates to
createFromConfig()as expectedThe migration is consistent and safe to merge.
src/Cms/Repositories/DatabaseCategoryRepository.php (1)
7-7: Settings namespace update is consistentSwitching to
Neuron\Data\Settings\SettingManagerhere matches the broader Settings namespace refactor and should be a no‑op at runtime as long as the updated neuron‑php/data dependency is installed.Please run the test suite after
composer updatewith the new dependencies to confirm there are no autoloading or wiring issues aroundSettingManager.tests/Cms/Repositories/DatabaseUserRepositoryTest.php (1)
8-8: Updated SettingManager import in tests looks goodUsing
Neuron\Data\Settings\SettingManagerin the mocks keeps the test suite aligned with the refactored Settings namespace; behavior remains unchanged because the class is mocked.After updating dependencies, please run this test class to ensure the new namespace resolves correctly under your autoloader.
src/Cms/Repositories/DatabasePostRepository.php (1)
10-10: Settings namespace import updated correctlyReferencing
Neuron\Data\Settings\SettingManagerhere keeps this repository in sync with the Settings refactor and ConnectionFactory usage; the constructor contract remains the same.After dependency updates, please run a small smoke test that instantiates
DatabasePostRepositoryvia your usual bootstrap path to confirm the new namespace resolves as expected.src/Cms/Email/helpers.php (1)
4-4: Email helpers now reference the new Settings namespaceUpdating the type to
Neuron\Data\Settings\ISettingSourcekeeps these helpers compatible with the refactored Settings package without changing their runtime behavior.Please ensure the updated Settings package is present in all environments where these helpers are used (including CLI contexts) and that any existing call sites still pass a compatible settings source.
resources/database/migrate/20251119224525_add_content_raw_to_posts.php (1)
13-22: Migration forcontent_rawis straightforward; verify adapter support for optionsAdding
content_rawas non‑nullable TEXT with a default empty Editor.js document is reasonable and should keep existing rows valid.The use of
'after' => 'body'and'comment' => 'Editor.js JSON content'is typically MySQL‑specific in Phinx; depending on how strictly you target SQLite/PostgreSQL as well, those options may be ignored or could need adapter‑specific handling.Please confirm that this migration runs cleanly against all database engines you support (especially in CI and local dev). If you see adapter‑specific issues, you may want to conditionally set
after/commentor drop them for non‑MySQL adapters.src/Cms/Events/EmailVerifiedEvent.php (1)
24-38: EmailVerifiedEvent implementation looks solidThe event cleanly encapsulates the verified
Uservia a readonly promoted property and exposes a clear, stable name ('email.verified') for subscribers. The design is consistent with the other event classes in this PR.src/Cms/Events/MaintenanceModeEnabledEvent.php (1)
24-40: MaintenanceModeEnabledEvent is consistent with the event modelReadonly constructor‑promoted properties for
enabledByandmessageplus a stable name'maintenance.enabled'match the existing disabled‑event pattern and give listeners all relevant context.src/Cms/Events/UserLoginEvent.php (1)
24-42: UserLoginEvent structure is consistent and ready for consumersThe event payload (User, IP, timestamp) and name
user.loginalign with the other event types you’ve added and give downstream listeners enough context without extra coupling. No issues spotted.src/Cms/Controllers/Content.php (2)
3-4: Version loading via factory is a clean upgradeSwitching to
Factories\Version::fromFile("../.version.json")while keeping the broad\Exceptioncatch preserves existing behavior and centralizes version deserialization in the data library. As long as the working directory is stable for web/CLI entry points, this looks good.Also applies to: 51-55, 85-94
201-213: Defaulting markdown page route toindeximproves resilienceUsing
$request->getRouteParameter('page') ?? 'index'avoids failures when the route omitspageand matches the typical “root markdown page” expectation. Just confirm your router/front-controller has anindex.md(or equivalent) to back this default.src/Cms/Repositories/DatabasePasswordResetTokenRepository.php (1)
5-8: Settings namespace refactor is consistentUsing
Neuron\Data\Settings\SettingManagerhere matches the broader Settings namespace migration and keeps the existing constructor contract intact. No repository behavior changes introduced.Also applies to: 23-32
tests/Cms/Services/PasswordResetterTest.php (1)
11-13: Test settings imports now align with production namespacesUpdating to
Settings\Source\MemoryandSettings\SettingManagerkeeps the test configuration in sync with the data layer refactor. The in-memory setup logic remains the same and should continue to exercise PasswordResetter as before.Also applies to: 34-42
tests/Cms/Services/EmailVerifierTest.php (1)
10-12: EmailVerifier tests correctly track the Settings namespace changeThe move to
Neuron\Data\Settings\Source\MemoryandSettings\SettingManagermirrors the production code and leaves the test behavior intact (test-mode email, same keys). Looks good.Also applies to: 31-39
resources/app/Initializers/RegistrationInitializer.php (1)
31-38: Registration initializer now guards against the correct Settings manager typeSwitching the instanceof check to
\Neuron\Data\Settings\SettingManagermatches the new Settings namespace and preserves the early-exit behavior when configuration isn’t wired. No additional issues here.src/Cms/Services/Auth/EmailVerifier.php (1)
10-11: I'll help you verify the review comment about email verification event emission safety. Let me start by exploring the codebase to understand how the Event::emit() method handles missing emitters and listener failures.
<function_calls>
#!/bin/bashFind the Event class definition
rg -n "class Event" --type=php
Find emit method definitions
rg -n "function emit|public function emit" --type=php -A 5
</function_calls>src/Cms/Services/Post/Creator.php (1)
35-40: Confirmed: API change is complete and well-implemented internally, but lacks external documentationThe migration is thorough within the repository:
- Post model correctly implements
setContent()with EditorJS JSON, auto-extracting plain text to_bodyfor backward compatibility- Creator service properly uses
setContent()- Admin form is fully updated with Editor.js frontend that serializes to EditorJS JSON
- Database schema includes
content_rawcolumn for JSON storage- Tests pass EditorJS JSON correctly
However, this package is publicly distributed via Composer (
"neuron-php/cms"library type), making Creator::create() a public API. The breaking change—requiring EditorJS JSON instead of rendered body strings—lacks a migration guide or changelog. No backward compatibility wrapper exists.tests/Cms/Services/RegistrationServiceTest.php (1)
10-11: LGTM! Namespace migration applied correctly.The import paths have been updated to use the new
Neuron\Data\Settingsnamespace, aligning with the project-wide refactoring.resources/app/Initializers/PasswordResetInitializer.php (1)
9-9: LGTM! Namespace migration applied correctly.The import path has been updated to use the new
Neuron\Data\Settings\SettingManagernamespace.src/Cms/Cli/Commands/Install/InstallCommand.php (1)
9-10: LGTM! Namespace migration applied correctly.The import paths have been updated to use the new
Neuron\Data\Settingsnamespace.tests/Cms/Services/AuthenticationTest.php (1)
11-11: LGTM! Namespace migration applied correctly.The import path has been updated to use the new
Neuron\Data\Settings\SettingManagernamespace.tests/Cms/BlogControllerTest.php (1)
13-14: LGTM! Namespace migration applied correctly.The import paths have been updated to use the new
Neuron\Data\Settingsnamespace.resources/app/Initializers/AuthInitializer.php (1)
32-32: LGTM! Type check updated for namespace migration.The instanceof check has been updated to reference the new
Neuron\Data\Settings\SettingManagernamespace.resources/views/blog/show.php (2)
51-160: LGTM! Comprehensive styling for EditorJS content.The CSS additions provide proper styling for post content and EditorJS widgets, including headings, lists, blockquotes, code blocks, images, and custom widgets.
39-39: I'll help you verify the concerns raised in the review comment. Let me search the codebase for the Blog controller and examine how$renderedContentis set.
<function_calls>
#!/bin/bashFind the Blog controller
fd -t f -i "blog" --type=php | head -20
</function_calls>Let me search more specifically for the Blog controller:
<function_calls>
#!/bin/bashSearch for Blog controller class definition
rg -n "class Blog" --type=php -A 5
</function_calls>Now let me search for where the show view is rendered:
<function_calls>
#!/bin/bashSearch for show view rendering and renderedContent variable
rg -n "renderedContent|show.php|blog/show" --type=php -B 3 -A 3
</function_calls>Let me also check the structure of the resources/views/blog directory:
<function_calls>
#!/bin/bashList files in blog views directory
fd -t f "show.php" && echo "---" && fd -t f "blog" resources/views/ 2>/dev/null | head -10
</function_calls>Let me check the full repository structure to understand the layout:
<function_calls>
#!/bin/bashGet an overview of the repository structure
git ls-files | grep -E "(controller|blog)" | head -20
</function_calls>src/Cms/Services/Auth/PasswordResetter.php (1)
10-10: LGTM! Namespace migration applied correctly.The import path has been updated to use the new
Neuron\Data\Settings\SettingManagernamespace.resources/app/Initializers/MaintenanceInitializer.php (1)
45-59: Updated Settings manager namespace check looks correctSwitching the instanceof check to
\Neuron\Data\Settings\SettingManageraligns this initializer with the new Settings namespace without changing runtime behavior. No further changes needed here.src/Cms/Events/PasswordResetCompletedEvent.php (1)
24-40: PasswordResetCompletedEvent structure and naming are consistentReadonly payload (User + IP) and the
password.reset_completedname match the existing event style in this module. Good to go.src/Cms/Events/PasswordResetRequestedEvent.php (1)
24-40: PasswordResetRequestedEvent aligns with the existing event modelReadonly User + IP payload and the
password.reset_requestedname follow the same pattern as your other events. Looks good as a foundation for security monitoring and auditing.src/Cms/Events/UserLogoutEvent.php (1)
1-41: LGTM!The event class is well-structured with clear documentation, follows the established event pattern, and properly uses constructor property promotion. The inclusion of
sessionDurationis a valuable addition for analytics.src/Cms/Controllers/Blog.php (1)
40-44: LGTM! Well-structured dependency initialization.The renderer initialization properly chains dependencies (WidgetRenderer → ShortcodeParser → EditorJsRenderer), creating a clean content processing pipeline.
src/Cms/Events/UserLoginFailedEvent.php (1)
1-44: LGTM!The event class is well-designed for security monitoring and audit trails. The inclusion of
reason,ip, andtimestampprovides comprehensive context for failed login attempts, which is essential for security analysis and compliance.src/Cms/Events/MaintenanceModeDisabledEvent.php (1)
8-39: LGTM! Event structure is appropriate.The event class is well-documented and follows the established pattern. Using a string for
$disabledBy(username/identifier) is consistent with the EnabledEvent and provides flexibility.src/Cms/Services/Post/Updater.php (1)
39-62: LGTM! Clean migration to EditorJS content handling.The parameter rename from
$bodyto$contentand the corresponding method call change fromsetBody()tosetContent()properly reflect the shift to EditorJS JSON format. The updated docblock clearly documents this change.src/Cms/Models/Post.php (3)
156-169: LGTM! Good error handling for JSON encoding.The
setContentArray()method properly handles JSON encoding failures with an explicit exception, which is better than silent failures. The use of\JsonExceptionis appropriate.
501-523: Well-designed content handling in fromArray().The method properly handles both
content_rawandcontentkeys, and supports both string and array formats for each. This provides good flexibility for different data sources while maintaining backward compatibility.
625-635: Use match expression correctly handles EditorJS block types.The match expression properly extracts text from various EditorJS block types. The handling of list items with
implode()and the fallback to empty string for unknown types is appropriate.resources/views/admin/posts/create.php (1)
104-108: The image upload endpoint handler for/admin/upload/imageis not implemented in the codebase.While the codebase has a robust CSRF protection system in place (CsrfFilter and CsrfToken service), the actual route handler for the
/admin/upload/imageendpoint referenced in the frontend editor configuration (create.php and edit.php) could not be located. The endpoint is configured in the views but no corresponding PHP route handler exists insrc/Cms/Controllers/or elsewhere.Recommendation: Implement the
/admin/upload/imageroute handler and ensure it is protected with the CsrfFilter to validate CSRF tokens before processing uploads.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Cms/Repositories/DatabaseUserRepository.php (1)
20-32: PDO connection is created but never used.The
$_pdoproperty is initialized viaConnectionFactory::createFromSettings()but no method in this class uses it. This wastes a database connection. Either remove the PDO initialization or add a comment explaining if it's needed for future use or external access.- private ?PDO $_pdo = null; - /** * Constructor * * @param SettingManager $settings Settings manager with database configuration * @throws Exception if database configuration is missing or adapter is unsupported */ public function __construct( SettingManager $settings ) { - // Keep PDO property for backwards compatibility with tests - $this->_pdo = ConnectionFactory::createFromSettings( $settings ); + // ORM is initialized in Bootstrap - no direct PDO needed }src/Cms/Repositories/DatabasePostRepository.php (1)
260-282:getByCategoryandgetByTagdon't load relations.Unlike
findByIdandfindBySlug, these methods return posts without loading categories and tags. This inconsistency may cause issues when consuming code expects relations to be populated.- return array_map( fn( $row ) => Post::fromArray( $row ), $rows ); + $posts = array_map( fn( $row ) => Post::fromArray( $row ), $rows ); + foreach( $posts as $post ) + { + $this->loadRelations( $post ); + } + return $posts;Apply to both
getByCategoryandgetByTag. Note: this introduces N+1 queries; consider batching if performance is critical.Also applies to: 287-309
♻️ Duplicate comments (1)
resources/views/admin/posts/create.php (1)
99-106: Plugin load check is incomplete (same issue as edit.php).Similar to
edit.php, the plugin availability check only validatesEditorJS,Header, andList, but the configuration uses additional plugins (ImageTool,Quote,CodeTool,Delimiter,RawTool).Apply the same fix suggested for
edit.phpto ensure all required plugins are validated before initialization.
🧹 Nitpick comments (17)
resources/config/neuron.yaml.example (4)
79-90: Cloudinary config is clear; add a note about handling credentials securelyThe structure and defaults look good and match typical Cloudinary usage. To avoid people committing real secrets into VCS, it’s worth explicitly calling that out in the example.
Consider this small tweak:
cloudinary: cloud_name: your-cloud-name # Your Cloudinary cloud name (required) api_key: your-api-key # API key from Cloudinary console (required) - api_secret: your-api-secret # API secret from Cloudinary console (required) + api_secret: your-api-secret # API secret from Cloudinary console (required) + # NOTE: In production, do not commit real Cloudinary credentials to version control. + # Prefer environment-specific config or environment variables for secrets. folder: neuron-cms/images # Default folder for uploads (optional) max_file_size: 5242880 # Maximum file size in bytes (5MB default) allowed_formats: [jpg, jpeg, png, gif, webp] # Allowed image formatsAlso double‑check that
max_file_sizeandallowed_formatsalign with whatever validation the uploader middleware/enforcer is using so the UX is consistent.
91-101: Database example looks fine; verify adapter strings match actual driver identifiersThe keys and comments are straightforward and useful. The only potential footgun is the
adapterstring values and how SQLite is meant to be configured.
- Confirm that
mysql,pgsql, andsqliteexactly match what your DB layer expects (nopdo_mysql/pdo_pgsqlvs custom names mismatch).- For
sqlite,host,port,user, andpassare typically ignored; consider adding a brief comment or a second example to show what a SQLite config should look like to avoid confusion.
110-128: Email/PHPMailer config is well documented; double‑check driver mapping and test mode behaviorThe email section is nicely annotated and gives practical SMTP examples. The main risk is drift between these keys and the actual mailer implementation.
- Ensure that
driver: mail|sendmail|smtpmaps cleanly to the PHPMailer setup (or equivalent) so users don’t hit silent fallbacks.- Confirm that
test_modebehaves exactly as described (log but do not send) across all drivers.- Optionally, you might add a short note that Gmail typically requires an app password and 2FA, which you already hint at via
your-app-passwordbut could call out explicitly in a comment.
129-138: Maintenance config is solid; consider clarifying IP/CIDR supportDefaults here look good (localhost allowlist, clear retry semantics, and optional custom view).
- If the implementation supports CIDR ranges and IPv6 ranges, it may be worth turning the commented
192.168.1.0/24example into a one‑line explanation in the comment to make that capability explicit.- Double‑check that
retry_afteris indeed interpreted as seconds (matching HTTPRetry-After) and thatcustom_viewpaths are resolved consistently with the rest of the view system..github/workflows/ci.yml (1)
10-11: Consider updatingactions/checkoutto v4.
actions/checkout@v3is functional but v4 is the current version with performance improvements and Node.js 20 runtime.- name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4resources/database/migrate/20251205000000_add_two_factor_and_timezone_to_users.php (1)
45-45:update()is called unconditionally.When both columns already exist,
update()is called without any pending changes. While harmless, you could wrap it in a conditional to avoid the no-op database call.- $table->update(); + if( !$table->hasColumn( 'two_factor_recovery_codes' ) || !$table->hasColumn( 'timezone' ) ) + { + $table->update(); + }Alternatively, track whether any column was added and only call
update()if needed.src/Cms/Repositories/DatabasePageRepository.php (1)
24-27: Constructor parameter is unused.The
$settingsparameter is no longer used since ORM initialization moved to Bootstrap. Consider either removing the parameter (breaking change) or documenting why it's retained for interface compatibility.src/Cms/Repositories/DatabasePostRepository.php (1)
411-426: Duplicate code:attachCategories/attachTagsoverlap with sync methods.The attach methods duplicate the INSERT logic from
syncCategories/syncTags. Consider refactoring to have sync call a shared insert helper, or remove the attach methods if sync is the canonical approach.Also applies to: 442-457
resources/views/admin/posts/edit.php (1)
119-126: Plugin load check is incomplete.The interval only checks for
EditorJS,Header, andList, but the editor configuration also usesImageTool,Quote,CodeTool,Delimiter, andRawTool. If any of these fail to load, the editor will initialize but those tools will be unavailable or cause errors.Consider extending the check to include all required plugins:
if (typeof EditorJS !== 'undefined' && typeof Header !== 'undefined' && - typeof List !== 'undefined') { + typeof List !== 'undefined' && + typeof ImageTool !== 'undefined' && + typeof Quote !== 'undefined' && + typeof CodeTool !== 'undefined' && + typeof Delimiter !== 'undefined' && + typeof RawTool !== 'undefined') {resources/views/admin/posts/create.php (1)
118-194: Consider extracting shared EditorJS initialization to a reusable script.The EditorJS initialization logic is nearly identical to
edit.php. While acceptable for view files, extracting the shared initialization code to a dedicated JavaScript file (e.g.,/js/admin/editor-init.js) would reduce duplication and simplify maintenance.src/Cms/Models/Post.php (1)
156-169: Proper error handling in setContentArray.Using
json_last_error_msg()and throwingJsonExceptionprovides clear feedback when JSON encoding fails. However, using theJSON_THROW_ON_ERRORflag withjson_encode()would be more idiomatic for PHP 7.3+.public function setContentArray( array $content ): self { - $encoded = json_encode( $content ); - - if( $encoded === false ) - { - $error = json_last_error_msg(); - throw new \JsonException( "Failed to encode content array to JSON: {$error}" ); - } + $encoded = json_encode( $content, JSON_THROW_ON_ERROR ); $this->_contentRaw = $encoded; $this->_body = $this->extractPlainText( $encoded ); return $this; }src/Cms/Services/Media/MediaValidator.php (2)
108-150: MIME type allowlist is hardcoded and doesn't align with configurableallowed_formats.The
allowed_formatssetting is configurable and used for extension validation, but the$allowedMimeTypesarray is hardcoded. If someone configures additional formats (e.g.,svg,bmp), the MIME check will reject them even though the extension check passes. Also,image/jpgis not a valid MIME type (should only beimage/jpeg).Consider deriving the MIME types from the allowed formats or making both configurable:
- $allowedMimeTypes = [ - 'image/jpeg', - 'image/jpg', - 'image/png', - 'image/gif', - 'image/webp' - ]; + $mimeTypeMap = [ + 'jpg' => 'image/jpeg', + 'jpeg' => 'image/jpeg', + 'png' => 'image/png', + 'gif' => 'image/gif', + 'webp' => 'image/webp' + ]; + + $allowedMimeTypes = array_values( array_intersect_key( $mimeTypeMap, array_flip( $allowedFormats ) ) );
81-99: Zero-byte check should precede max-size check.A zero-byte file will trigger both the
$size > $maxSizecondition (false) and then the$size === 0condition. While logically correct, checking for zero-byte first provides a more specific error message and is slightly more efficient.private function validateFileSize( int $size ): bool { + if( $size === 0 ) + { + $this->_errors[] = 'File is empty'; + return false; + } + $maxSize = $this->_settings->get( 'cloudinary', 'max_file_size' ) ?? 5242880; // 5MB default if( $size > $maxSize ) { $maxSizeMB = round( $maxSize / 1048576, 2 ); $this->_errors[] = "File size exceeds maximum allowed size of {$maxSizeMB}MB"; return false; } - if( $size === 0 ) - { - $this->_errors[] = 'File is empty'; - return false; - } - return true; }tests/Cms/Services/Media/MediaValidatorTest.php (1)
163-167: Consider adding a test for valid image upload and MIME type mismatch.The comment acknowledges that full validation with real image files is complex, but you could use a minimal valid JPEG/PNG byte sequence to test the happy path. Additionally, testing a file with a valid extension but invalid MIME type (e.g., a
.jpgfile containing text) would improve coverage.Would you like me to provide example code for creating a minimal valid image in vfsStream for the happy path test?
src/Cms/Controllers/Admin/Media.php (2)
133-144: Usingexitmakes the controller difficult to unit test.The
exitcalls prevent proper testing and don't align well with MVC patterns. Consider returning early or throwing an exception that the framework catches, or using a response object pattern.If refactoring is not feasible now, you could at least extract the exit to a protected method that can be mocked in tests:
+ /** + * Terminate the response (can be overridden in tests) + */ + protected function terminate(): void + { + exit; + } + private function returnEditorJsSuccess( array $result ): void { echo json_encode( [ 'success' => 1, 'file' => [ 'url' => $result['url'], 'width' => $result['width'], 'height' => $result['height'] ] ] ); - exit; + $this->terminate(); }Also applies to: 168-175
49-82: Consider extracting common upload logic to reduce duplication.Both
uploadImageanduploadFeaturedImageshare the same validation and upload logic, differing only in response format. This could be refactored to a private helper.private function processUpload(): array { if( !isset( $_FILES['image'] ) ) { throw new \InvalidArgumentException( 'No file was uploaded' ); } $file = $_FILES['image']; if( !$this->_validator->validate( $file ) ) { throw new \InvalidArgumentException( $this->_validator->getFirstError() ); } return $this->_uploader->upload( $file['tmp_name'] ); }Also applies to: 92-125
src/Cms/Cli/Commands/Install/UpgradeCommand.php (1)
424-437:runMigrations()is misleading - it doesn't run migrations.The method name suggests it runs migrations, but it only outputs instructions. Either rename it to
displayMigrationInstructions()or implement actual migration execution.- private function runMigrations(): bool + private function displayMigrationInstructions(): void { - $this->output->writeln( "Running migrations...\n" ); - - // For now, instruct user to run migrations manually - // In future, could integrate with MigrationManager - $this->output->info( "Run: php neuron db:migrate" ); - - return true; }Update the call site at line 145 accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
.github/workflows/ci.yml(1 hunks).gitignore(1 hunks)MIGRATIONS.md(1 hunks)UPGRADE_NOTES.md(1 hunks)composer.json(2 hunks)readme.md(1 hunks)resources/.cms-manifest.json(1 hunks)resources/config/database.yaml.example(0 hunks)resources/config/email.yaml.example(0 hunks)resources/config/maintenance.yaml.example(0 hunks)resources/config/neuron.yaml.example(1 hunks)resources/config/routes.yaml(1 hunks)resources/database/migrate/20250111000000_create_users_table.php(1 hunks)resources/database/migrate/20251205000000_add_two_factor_and_timezone_to_users.php(1 hunks)resources/views/admin/posts/create.php(3 hunks)resources/views/admin/posts/edit.php(3 hunks)src/Cms/Cli/Commands/Install/InstallCommand.php(2 hunks)src/Cms/Cli/Commands/Install/UpgradeCommand.php(1 hunks)src/Cms/Cli/Provider.php(1 hunks)src/Cms/Controllers/Admin/Media.php(1 hunks)src/Cms/Dtos/MediaUploadDto.yaml(1 hunks)src/Cms/Models/Post.php(5 hunks)src/Cms/Repositories/DatabaseCategoryRepository.php(7 hunks)src/Cms/Repositories/DatabasePageRepository.php(5 hunks)src/Cms/Repositories/DatabasePostRepository.php(13 hunks)src/Cms/Repositories/DatabaseTagRepository.php(6 hunks)src/Cms/Repositories/DatabaseUserRepository.php(5 hunks)src/Cms/Services/Media/CloudinaryUploader.php(1 hunks)src/Cms/Services/Media/IMediaUploader.php(1 hunks)src/Cms/Services/Media/MediaValidator.php(1 hunks)tests/Cms/BlogControllerTest.php(3 hunks)tests/Cms/Repositories/DatabaseCategoryRepositoryTest.php(2 hunks)tests/Cms/Repositories/DatabasePostRepositoryTest.php(3 hunks)tests/Cms/Repositories/DatabaseTagRepositoryTest.php(2 hunks)tests/Cms/Repositories/DatabaseUserRepositoryTest.php(3 hunks)tests/Cms/Services/AuthenticationTest.php(3 hunks)tests/Cms/Services/Media/CloudinaryUploaderTest.php(1 hunks)tests/Cms/Services/Media/MediaValidatorTest.php(1 hunks)
💤 Files with no reviewable changes (3)
- resources/config/maintenance.yaml.example
- resources/config/email.yaml.example
- resources/config/database.yaml.example
✅ Files skipped from review due to trivial changes (2)
- resources/.cms-manifest.json
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- composer.json
🧰 Additional context used
🧬 Code graph analysis (13)
resources/views/admin/posts/edit.php (2)
src/Cms/View/helpers.php (1)
route_path(70-79)src/Cms/Models/Post.php (1)
getContentRaw(133-136)
src/Cms/Services/Media/IMediaUploader.php (2)
src/Cms/Controllers/Admin/Media.php (1)
Media(18-192)src/Cms/Services/Media/CloudinaryUploader.php (3)
upload(67-88)uploadFromUrl(98-120)delete(129-142)
src/Cms/Services/Media/MediaValidator.php (2)
src/Cms/Controllers/Admin/Media.php (2)
Media(18-192)__construct(26-39)src/Cms/Services/Media/CloudinaryUploader.php (1)
__construct(27-31)
src/Cms/Controllers/Admin/Media.php (3)
src/Cms/Services/Media/CloudinaryUploader.php (3)
CloudinaryUploader(16-197)__construct(27-31)upload(67-88)src/Cms/Services/Media/MediaValidator.php (3)
MediaValidator(14-192)__construct(24-27)getFirstError(188-191)src/Cms/Services/Media/IMediaUploader.php (1)
upload(22-22)
tests/Cms/Services/Media/CloudinaryUploaderTest.php (2)
src/Cms/Services/Media/CloudinaryUploader.php (3)
CloudinaryUploader(16-197)upload(67-88)uploadFromUrl(98-120)src/Cms/Services/Media/IMediaUploader.php (2)
upload(22-22)uploadFromUrl(32-32)
src/Cms/Repositories/DatabasePostRepository.php (5)
src/Cms/Models/Post.php (9)
fromArray(488-585)getId(57-60)setCategories(376-380)setTags(436-440)toArray(592-609)setId(65-69)getCategories(366-369)getTags(426-429)incrementViewCount(321-325)src/Cms/Models/Category.php (4)
fromArray(143-175)getId(37-40)toArray(182-192)setId(45-49)src/Cms/Models/Tag.php (4)
fromArray(125-156)getId(36-39)toArray(163-172)setId(44-48)src/Cms/Models/User.php (4)
fromArray(438-489)getId(61-64)toArray(413-433)setId(69-73)src/Cms/Repositories/IPostRepository.php (9)
findBySlug(22-22)create(27-27)count(104-104)delete(37-37)all(47-47)getByAuthor(56-56)getByCategory(65-65)getByTag(74-74)incrementViewCount(109-109)
src/Cms/Services/Media/CloudinaryUploader.php (3)
src/Cms/Controllers/Admin/Media.php (2)
Media(18-192)__construct(26-39)src/Cms/Services/Media/MediaValidator.php (1)
__construct(24-27)src/Cms/Services/Media/IMediaUploader.php (3)
upload(22-22)uploadFromUrl(32-32)delete(41-41)
src/Cms/Repositories/DatabaseUserRepository.php (4)
src/Cms/Repositories/DatabaseCategoryRepository.php (4)
create(77-98)delete(131-137)all(142-145)count(150-153)src/Cms/Repositories/DatabasePostRepository.php (4)
create(115-144)delete(212-218)all(223-240)count(338-348)src/Cms/Repositories/DatabaseTagRepository.php (4)
create(61-82)delete(115-121)all(126-129)count(134-137)src/Cms/Models/User.php (3)
toArray(413-433)setId(69-73)getId(61-64)
src/Cms/Repositories/DatabaseCategoryRepository.php (2)
src/Cms/Repositories/DatabaseTagRepository.php (4)
create(61-82)all(126-129)count(134-137)allWithPostCount(142-163)src/Cms/Models/Category.php (3)
toArray(182-192)setId(45-49)getId(37-40)
src/Cms/Cli/Commands/Install/UpgradeCommand.php (1)
src/Cms/Cli/Commands/Install/InstallCommand.php (2)
__construct(25-32)getName(37-40)
tests/Cms/Services/Media/MediaValidatorTest.php (1)
src/Cms/Services/Media/MediaValidator.php (4)
MediaValidator(14-192)validate(35-73)getErrors(178-181)getFirstError(188-191)
src/Cms/Repositories/DatabaseTagRepository.php (1)
src/Cms/Models/Tag.php (3)
toArray(163-172)setId(44-48)getId(36-39)
resources/views/admin/posts/create.php (1)
src/Cms/View/helpers.php (1)
route_path(70-79)
🪛 actionlint (1.7.9)
.github/workflows/ci.yml
27-27: the runner of "codecov/codecov-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 LanguageTool
UPGRADE_NOTES.md
[style] ~44-~44: The double modal “Required Run” is nonstandard (only accepted in certain dialects). Consider “to be Run”.
Context: ... Changes - None ### Action Required 1. Run php neuron cms:upgrade to copy new mi...
(NEEDS_FIXED)
🪛 PHPMD (2.15.0)
src/Cms/Cli/Commands/Install/UpgradeCommand.php
61-61: Avoid unused parameters such as '$parameters'. (undefined)
(UnusedFormalParameter)
406-406: Avoid unused local variables such as '$installedMigrations'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (55)
src/Cms/Cli/Commands/Install/InstallCommand.php (2)
9-10: Namespace migration verified as complete.No references to the old
Neuron\Data\Setting\...namespace remain in the codebase. The migration toNeuron\Data\Settings\...is consistently applied across all importing files, including multiple repositories, services, and tests. The imports at lines 9-10 are correct and aligned with the rest of the codebase.
185-186: The upload directories are unused; files are uploaded to Cloudinary, not stored locally.The
/storage/uploadsand/storage/uploads/tempdirectories are created during installation but are never used. The actual upload flow validates files viaMediaValidator(which checks MIME type, extension, and image integrity) and then uploads directly to Cloudinary. No local file storage, caching, or temporary handling occurs.The security concerns and cleanup mechanisms mentioned in the original review are not applicable since files don't reside on disk. If these directories are intended for future local storage fallback or caching, that capability should be implemented and explicitly documented.
Likely an incorrect or invalid review comment.
resources/config/neuron.yaml.example (1)
102-108: Migrations config: ensure paths/table name match the migration tool’s expectationsThe
migrationsblock is sensible and clearly documented. Correctness depends on how your migration tooling is wired.
- Verify that
path,seeds_path, andschema_fileare resolved relative to project root in the same way the migration runner expects.- Confirm that
table: phinx_logis consistent with how your migration library is configured elsewhere (many Phinx setups usephinxlog), or that you’ve intentionally standardized on this custom name.- Make sure
auto_dump_schemais wired into the migration command so toggling it here has the intended effect.readme.md (1)
2-2: LGTM!The Codecov badge addition aligns well with the CI workflow changes that now upload coverage reports.
.github/workflows/ci.yml (1)
13-24: LGTM!The PHP setup with pcov for coverage and the PHPUnit configuration are well-structured. Using
--coverage-filter srcensures only source code is measured.src/Cms/Repositories/DatabaseCategoryRepository.php (8)
37-40: LGTM!Clean ORM usage for the
findByIdmethod.
45-56: LGTM!The
findBySlugandfindByNamemethods correctly use the ORM's fluent query builder pattern.
64-72: LGTM!Good defensive check for empty array before querying, and proper use of
whereInfor batch lookups.
103-126: LGTM!The update method properly handles duplicate detection while excluding the current record, and uses the ORM's
save()method correctly.
131-137: LGTM!The delete method correctly uses the query builder and returns a meaningful boolean result.
142-153: LGTM!The
all()andcount()methods are concise and leverage the ORM effectively.
158-179: LGTM!The raw SQL approach for the JOIN + aggregation query is consistent with
DatabaseTagRepository::allWithPostCount(). The TODO comment appropriately documents the ORM limitation.
77-98: The code is correct as-is. WhiletoArray()does return dates as'Y-m-d H:i:s'strings, the Category model'sfromArray()method explicitly handles string-to-DateTimeImmutable conversion (lines 142-158), which the ORM'screate()method uses internally. Tests confirm this works correctly without any changes needed.src/Cms/Repositories/DatabasePageRepository.php (1)
32-36: LGTM on eager loading for author.Using
Page::with('author')for eager loading is the correct approach to avoid N+1 queries when accessing author data.src/Cms/Repositories/DatabaseUserRepository.php (2)
133-136: ORM query pattern looks correct.The
User::orderBy('created_at', 'DESC')->all()pattern is consistent with other repositories and provides expected ordering.
83-88: No action needed; password hashing is handled correctly.The password is already hashed in the service layer (via
PasswordHasher) before being set on the User model throughsetPasswordHash(). The User model'ssetPasswordHash()method (line 120-124) simply stores the hash value without modification. WhenUser::create($user->toArray())is called, it persists the pre-computed hash as-is; there is no re-hashing mechanism in the model or ORM layer.src/Cms/Repositories/DatabaseTagRepository.php (3)
142-163: Raw SQL forallWithPostCount()is appropriate.Keeping raw SQL for the JOIN with aggregation is the right pragmatic choice. The TODO comment clearly indicates future work.
108-109: Ensuresave()is called on an ORM-hydrated instance.If
$tagwas instantiated vianew Tag()rather than fetched from the database,save()may not work correctly depending on ORM implementation. Verify this path is only reached with ORM-loaded entities.
75-79: This pattern is standard and working across all models in the codebase.The concern about passing null
idintoArray()tocreate()is unfounded. The exact same pattern (Model::create($entity->toArray())) is used consistently across all repositories (User, Post, Page, Category, Tag) and is functioning properly. The Neuron ORM (v0.1.*) handles null values in mass assignment gracefully, treatingnullvalues as absent fields that don't override database defaults like auto-increment IDs. No special filtering is needed.Likely an incorrect or invalid review comment.
src/Cms/Repositories/DatabasePostRepository.php (2)
163-196: Raw SQL UPDATE with ORM CREATE is intentional but worth documenting.The comment on lines 163-164 explains why raw SQL is used for updates. This asymmetry (ORM for create, raw SQL for update) is acceptable given the ORM limitation, but consider adding a TODO to revisit when the ORM supports dirty tracking.
76-110:loadRelationshelper is well-structured.The method cleanly separates loading categories and tags via JOINs. Good use of
array_mapwith arrow functions.resources/database/migrate/20251205000000_add_two_factor_and_timezone_to_users.php (1)
26-43: Theaftercolumn references are safe—remove unnecessary idempotency checks.The columns
two_factor_secretandlast_login_atare created in the initial migration, so theafterplacement is valid. However,two_factor_recovery_codesandtimezoneare already defined in20250111000000_create_users_table.php. ThehasColumn()guards make this migration safe for existing installations, but the column definitions are redundant for new installs. Either consolidate these columns into the initial migration, or clarify in code comments why this migration exists as a separate patch.resources/database/migrate/20250111000000_create_users_table.php (1)
25-25: LGTM! Verify recovery codes are encrypted at the application layer.The
two_factor_recovery_codescolumn is appropriately defined as nullable TEXT. Since recovery codes are sensitive authentication credentials, ensure they are encrypted before storage at the application layer.tests/Cms/Repositories/DatabaseTagRepositoryTest.php (1)
8-8: LGTM! ORM initialization follows consistent test pattern.The ORM initialization with
Model::setPdo()in setUp is consistent with the broader migration to ORM-based data access across repository tests.Also applies to: 33-34
tests/Cms/Repositories/DatabaseCategoryRepositoryTest.php (1)
8-8: LGTM! ORM initialization follows consistent test pattern.The ORM initialization matches the pattern used in other repository tests, supporting the migration to ORM-based data access.
Also applies to: 33-34
tests/Cms/Repositories/DatabaseUserRepositoryTest.php (1)
8-9: LGTM! Test updates align with ORM migration and 2FA support.The changes correctly reflect:
- Namespace migration for SettingManager
- ORM initialization consistent with other repository tests
- Schema update for
two_factor_recovery_codesmatching the migrationAlso applies to: 47-48, 66-66
src/Cms/Cli/Provider.php (1)
21-30: LGTM! Upgrade command registration follows CLI conventions.The new
cms:upgradecommand is properly registered using the same pattern as existing commands. The comment update to plural form is appropriate.src/Cms/Dtos/MediaUploadDto.yaml (1)
1-19: LGTM! Validation rules are appropriate for media uploads.The DTO configuration correctly defines:
- Required file field with NotEmpty validation
- Optional folder field with reasonable 255-character limit
tests/Cms/BlogControllerTest.php (3)
13-16: LGTM! ORM initialization and Settings migration applied correctly.The test setup now properly initializes the ORM and uses the migrated Settings namespace, consistent with other test files.
Also applies to: 54-55
100-120: LGTM! Users table creation supports blog author functionality.The addition of the users table in test setup is appropriate if the Blog controller needs to access author information. The schema correctly includes the new 2FA fields.
129-129: LGTM! Editor.js default structure is correctly formatted.The
content_rawcolumn default value'{"blocks":[]}'is a valid empty Editor.js document structure, supporting the block-based editor upgrade.tests/Cms/Services/Media/CloudinaryUploaderTest.php (3)
17-29: LGTM! Test setup correctly initializes Cloudinary configuration.The in-memory settings with test Cloudinary credentials are appropriate for unit testing the uploader service.
31-61: LGTM! Unit tests appropriately cover error paths.The tests correctly validate exception handling for:
- Missing configuration
- Non-existent files
- Invalid URLs
68-113: LGTM! Integration tests appropriately marked incomplete.Marking integration tests as incomplete is correct since they require actual Cloudinary credentials and external resources. The commented examples provide clear guidance for future integration testing.
UPGRADE_NOTES.md (1)
1-151: Well-structured upgrade documentation.The upgrade notes provide clear instructions for the upgrade workflow, version-specific changes, and troubleshooting guidance. The section headers and formatting are appropriate for developer documentation.
One consideration: ensure the version numbers (2025.12.5, 2025.11.7) align with actual release tags in the repository to avoid confusion.
tests/Cms/Repositories/DatabasePostRepositoryTest.php (2)
35-36: LGTM - ORM initialization in test setup.Initializing the ORM with the test PDO connection ensures the repository tests can properly exercise ORM-backed data access.
56-85: Schema updates align with migration requirements.The test schema correctly includes the new
two_factor_recovery_codes,timezonecolumns for users, andcontent_rawfor posts. The default'{"blocks":[]}'matches the Editor.js empty content structure used in the Post model.resources/views/admin/posts/edit.php (2)
93-103: Good XSS prevention in content loading.Using
JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOTflags ensures the JSON output is safe for embedding in a script context, preventing injection attacks via malicious post content.
199-211: Form submission pattern is correct.Calling
e.target.submit()aftere.preventDefault()correctly bypasses the event listener and performs a native form submission, avoiding an infinite loop.tests/Cms/Services/AuthenticationTest.php (2)
11-12: LGTM - Updated imports for ORM and Settings namespace.The namespace migration from
Neuron\Data\SettingtoNeuron\Data\Settingsand the addition of ORM initialization align with the broader refactoring in this PR.
50-51: ORM initialization added correctly.Initializing the ORM with the test PDO connection before creating tables ensures proper ORM functionality during authentication tests.
resources/views/admin/posts/create.php (1)
75-82: CDN versions are now pinned - previous feedback addressed.The script versions are pinned to specific versions (e.g.,
@2.29.1,@2.8.1), which addresses the previous review concern about using@latest.src/Cms/Services/Media/IMediaUploader.php (1)
12-42: Well-designed media uploader interface.The
IMediaUploaderinterface provides a clean contract for media operations with appropriate PHPDoc documentation. The return array structure (url,public_id,width,height,format) is well-documented and aligns with theCloudinaryUploaderimplementation shown in the relevant snippets.src/Cms/Models/Post.php (2)
21-22: Good design for dual content storage.Maintaining both
_body(plain text) and_contentRaw(Editor.js JSON) provides backward compatibility for code that expects plain text while supporting the new block-based editor format.
499-533: Content initialization in fromArray is well-structured.The logic correctly handles multiple input formats (
content_rawas string/array,contentas string/array) and properly falls back to extracting plain text whenbodyis not explicitly provided.src/Cms/Services/Media/MediaValidator.php (1)
14-73: LGTM!The overall validation flow is well-structured with proper early returns, error accumulation, and comprehensive checks including upload errors, file existence, size, and type validation. The use of
is_uploaded_file()could provide additional security for production environments, butfile_exists()is acceptable for general validation.tests/Cms/Services/Media/MediaValidatorTest.php (2)
94-111: Test relies on$file['size']from array, not actual file size.The test creates a file with
'fake content'(12 bytes) but passessize => 10485760in the array. This works becauseMediaValidatortrusts$file['size']from the$_FILESsuperglobal. This is correct behavior for testing the validator, but consider adding a comment clarifying that PHP populates$_FILES['size']at upload time.
20-34: LGTM!Good test setup with virtual filesystem and in-memory settings. The configuration matches the validator's expected defaults, enabling consistent test behavior.
src/Cms/Controllers/Admin/Media.php (1)
26-39: LGTM!Good defensive check for Settings in the constructor with a clear exception message. The dependency injection via Registry is appropriate for this framework.
src/Cms/Services/Media/CloudinaryUploader.php (3)
39-57: LGTM!Good validation of required Cloudinary configuration with a clear error message. The exception during construction ensures fast-fail behavior.
184-196: LGTM!Clean normalization of Cloudinary response with sensible defaults for missing fields. Preferring
secure_urloverurlis the correct approach.
67-88: LGTM!Good file existence validation before upload attempt, and proper exception chaining preserves the original error context.
src/Cms/Cli/Commands/Install/UpgradeCommand.php (3)
61-61: Unused$parametersis acceptable for interface compliance.The static analysis flags
$parametersas unused, but this is a common pattern for CLI command implementations where the method signature is dictated by the parent class. No action needed.
21-28: LGTM!Constructor follows the same pattern as
InstallCommand, usinggetcwd()for project path and relative path traversal for component path.
284-290: LGTM!Clean use of
array_diffto identify new migrations by comparing package and installed lists.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
26-33: Reconsiderfail_ci_if_error: truefor codecov upload.The codecov-action has been updated to v5 (addressing the prior review concern), but the
fail_ci_if_errorsetting differs from the previously suggested value. Setting this totruemeans your CI will fail if the codecov service experiences outages, network issues, or slowdowns—external factors outside your control.Industry best practice is to set this to
falseto prevent codecov availability issues from blocking merges. Consider reverting unless you have a specific requirement for codecov success to be mandatory.Apply this diff to align with best practices:
- name: Upload coverage to Codecov uses: codecov/codecov-action@v5 with: token: ${{ secrets.CODECOV_TOKEN }} files: ./coverage.xml flags: cms slug: Neuron-PHP/cms - fail_ci_if_error: true + fail_ci_if_error: false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(1 hunks)readme.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- readme.md
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
13-24: Modern CI setup with solid coverage configuration.The workflow updates to use
shivammathur/setup-php@v2with PHP 8.4 is a clean improvement over container-based execution. The Composer install flags (--optimize-autoloader, --no-interaction) and PHPUnit coverage command (with --coverage-clover and --coverage-filter src) are both well-configured and follow CI best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/Cms/Services/CsrfTokenTest.php (2)
59-73: Test name vs. implementation mismatch.The test name
testGenerateTokenIsDifferentEachTimesuggests verifying that callinggenerate()multiple times on the same instance produces different tokens. However, the test creates two separateCsrfTokeninstances with different seeds. This effectively tests that different seeds produce different tokens, not that successive calls produce different tokens.Consider renaming to
testDifferentInstancesWithDifferentSeedsProduceDifferentTokensor restructuring to test multiplegenerate()calls on a single instance (ifFakeRandomsupports advancing state per call).
179-186: Consider tightening the regex to match hex format.According to the
CsrfTokenimplementation, tokens are generated using$this->random->string(64, 'hex'), which produces lowercase hexadecimal characters (0-9, a-f). The current regex/^[a-zA-Z0-9]+$/is overly permissive.- $this->assertMatchesRegularExpression('/^[a-zA-Z0-9]+$/', $token); + $this->assertMatchesRegularExpression('/^[a-f0-9]+$/', $token);src/Cms/Services/Auth/CsrfToken.php (2)
6-7: Solid randomness DI; consider minor consistency & DI refinementsInjecting
IRandomwith aRealRandomdefault keeps existing call sites working and makes the class testable, which is great. Two small follow‑ups you might consider:
- Align the property name with the existing private member convention (
$_sessionManager,$_tokenKey) to keep style consistent (e.g.,private IRandom $_random;and$this->_random).- Instead of
new RealRandom()inside the constructor, you could let your DI container/wiring provide the default implementation, which keeps this service fully configuration-driven and avoids hard-coding a concrete in the service itself.These are non-blocking and can be deferred.
Also applies to: 21-27
34-34: ConfirmRealRandom::string()is CSPRNG-backed for CSRF tokensPreviously this used
random_bytes, which is explicitly CSPRNG-backed. CSRF tokens must stay cryptographically unpredictable, so please verify thatRealRandom::string(64, 'hex')is implemented on top of a cryptographically secure source and yields comparable entropy (effectively 64 hex chars ≈ 256 bits). IfIRandomcan be backed by weaker RNGs, consider tightening its contract or introducing a dedicated crypto-safe interface for security-sensitive use like CSRF.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Cms/Services/Auth/CsrfToken.php(2 hunks)tests/Cms/Services/CsrfTokenTest.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Cms/Services/Auth/CsrfToken.php (2)
src/Cms/Controllers/Member/Registration.php (1)
__construct(40-61)src/Cms/Controllers/Auth/Login.php (1)
__construct(30-44)
tests/Cms/Services/CsrfTokenTest.php (2)
src/Cms/Auth/SessionManager.php (1)
SessionManager(13-188)src/Cms/Services/Auth/CsrfToken.php (5)
CsrfToken(17-75)generate(32-37)regenerate(71-74)getToken(42-50)validate(55-66)
🔇 Additional comments (3)
tests/Cms/Services/CsrfTokenTest.php (3)
20-34: LGTM! Good use of dependency injection for deterministic testing.The setUp properly injects
FakeRandomintoCsrfToken, enabling reproducible token generation in tests. This is a solid pattern for testing randomness-dependent code.
142-157: LGTM! Regeneration behavior is properly tested.The test correctly verifies that
regenerate()produces a different token and updates the stored token. The seed manipulation approach effectively simulates the randomness that would occur in production.
159-177: LGTM! Token invalidation is properly verified.This test correctly ensures that after regeneration, the old token becomes invalid while the new token is accepted. Good coverage for security-critical CSRF behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Cms/Services/Auth/PasswordResetter.php (1)
112-116: Event emissions are well‑placed; consider decoupling IP/source retrieval as a small cleanupEmitting
PasswordResetRequestedEventandPasswordResetCompletedEventafter the respective operations, withUserand$_SERVER['REMOTE_ADDR'] ?? 'unknown', is sensible and should give useful audit signals.If you want to tighten design further, consider:
- Centralizing IP acquisition (e.g., a small helper or a request/context service) instead of reading
$_SERVERdirectly in multiple places.- Optionally routing events via an injected dispatcher instead of
Event::emitfor easier testing/mocking.These are architectural nits; current code is fine to ship.
Also applies to: 181-185
src/Cms/Services/Auth/EmailVerifier.php (2)
28-57: Random generator DI is solid; consider minor naming consistencyInjecting
?IRandom $randomand defaulting tonew RealRandom()mirrors the pattern used inCsrfTokenandPasswordResetter, improving testability while preserving secure defaults. The only minor nit is that this class uses$_randomwhereas others use$this->random; if you care about cross-service consistency/grepability, you might consider aligning the property name, but it’s not critical.
169-172: EmittingEmailVerifiedEventis good; consider isolating failures and importing classesEmitting
EmailVerifiedEventon successful verification is a nice integration point with the event bus. Two optional improvements:
- If
Event::emit()can throw or otherwise fail, consider wrapping this call in atry/catchand logging failures so that a broken listener doesn’t causeverifyEmail()to surface as an error after the user has already been activated.- For readability, you could add
use Neuron\Application\CrossCutting\Event;anduse Neuron\Cms\Events\EmailVerifiedEvent;at the top and callEvent::emit(new EmailVerifiedEvent($user));instead of using fully-qualified names inline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Cms/Services/Auth/EmailVerifier.php(5 hunks)src/Cms/Services/Auth/PasswordResetter.php(7 hunks)versionlog.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- versionlog.md
🧰 Additional context used
🧬 Code graph analysis (2)
src/Cms/Services/Auth/EmailVerifier.php (3)
src/Cms/Services/Auth/PasswordResetter.php (1)
__construct(45-62)src/Cms/Services/Auth/CsrfToken.php (1)
__construct(23-27)src/Cms/Events/EmailVerifiedEvent.php (1)
EmailVerifiedEvent(24-39)
src/Cms/Services/Auth/PasswordResetter.php (3)
src/Cms/Services/Auth/CsrfToken.php (1)
__construct(23-27)src/Cms/Events/PasswordResetRequestedEvent.php (1)
PasswordResetRequestedEvent(24-41)src/Cms/Events/PasswordResetCompletedEvent.php (1)
PasswordResetCompletedEvent(24-41)
🔇 Additional comments (3)
src/Cms/Services/Auth/PasswordResetter.php (1)
10-12: Randomness DI + token generation look correct and improve testabilityInjecting
IRandomwith aRealRandomfallback keeps the constructor backward‑compatible (new optional last parameter), aligns with theCsrfTokenpattern, and makes token generation testable. The switch to$this->_random->string(64, 'hex')still yields a 64‑char token which is then SHA‑256 hashed, so stored token shape remains consistent with the previous implementation.No changes needed from my side here.
Also applies to: 29-29, 43-43, 51-52, 61-61, 96-97
src/Cms/Services/Auth/EmailVerifier.php (2)
10-12: IRandom/RealRandom/SettingManager imports look correct and consistentImporting
Neuron\Core\System\IRandom/RealRandomalongsideNeuron\Data\Settings\SettingManagerkeeps this service aligned with the other auth components using the same abstractions and supports the new DI-based randomness pattern. No changes needed here.
83-86: Token generation remains strong; verify that RealRandom (from neuron-php/mvc) is backed by a cryptographically secure sourceThe change to use
$this->_random->string(64, 'hex')maintains strong security (64-char token = 32 bytes of entropy) and makes randomness pluggable for tests via theIRandominterface. The main consideration is thatRealRandomfrom the external neuron-php/mvc package must remain backed by a cryptographically secure source (e.g.,random_bytes()or/dev/urandom); otherwise, email-verification tokens could become guessable if an accidental implementation uses weak randomness in production. VerifyRealRandom's implementation in the neuron-php/mvc package to confirm it uses a cryptographically secure random source.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Cms/Services/Page/Creator.php (1)
21-27: Slug fallback now depends onIRandom::uniqueIdbeing slug‑safeThe fallback
'page-' . $this->_random->uniqueId()assumesuniqueId()only yields characters allowed by your slug rules. If any customIRandomimplementation ever returns characters outside[a-z0-9-], you could end up with invalid slugs since the normalization isn’t re-applied after the fallback.Either:
- Document/guarantee on
IRandom::uniqueId()that its output is slug-safe, or- Re-run the normalization on the fallback string (and mirror that change in the other
generateSlug()helpers using this pattern).Also applies to: 85-99
versionlog.md (1)
1-13: Consider adding a release date to the0.8.9headingEarlier entries (e.g.,
0.8.8 2025-11-16) include a date in the heading. Adding the date for0.8.9would keep the changelog format consistent and make it easier to track releases over time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Cms/Services/Category/Creator.php(4 hunks)src/Cms/Services/Category/Updater.php(4 hunks)src/Cms/Services/Page/Creator.php(4 hunks)src/Cms/Services/Post/Creator.php(6 hunks)src/Cms/Services/Post/Updater.php(6 hunks)src/Cms/Services/Tag/Creator.php(4 hunks)versionlog.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/Cms/Services/Tag/Creator.php (3)
src/Cms/Services/Category/Creator.php (1)
__construct(25-29)src/Cms/Services/Page/Creator.php (1)
__construct(23-27)src/Cms/Services/Post/Creator.php (1)
__construct(27-38)
src/Cms/Services/Category/Creator.php (4)
src/Cms/Services/Category/Updater.php (1)
__construct(25-29)src/Cms/Services/Page/Creator.php (1)
__construct(23-27)src/Cms/Services/Post/Creator.php (1)
__construct(27-38)src/Cms/Services/Tag/Creator.php (1)
__construct(22-26)
src/Cms/Services/Category/Updater.php (6)
src/Cms/Services/Category/Creator.php (1)
__construct(25-29)src/Cms/Services/Page/Creator.php (1)
__construct(23-27)src/Cms/Services/Post/Creator.php (1)
__construct(27-38)src/Cms/Services/Tag/Creator.php (1)
__construct(22-26)src/Cms/Services/Content/ShortcodeParser.php (1)
__construct(20-23)src/Cms/Services/Widget/WidgetRenderer.php (1)
__construct(21-24)
src/Cms/Services/Post/Updater.php (5)
src/Cms/Services/Category/Creator.php (1)
__construct(25-29)src/Cms/Services/Page/Creator.php (1)
__construct(23-27)src/Cms/Services/Post/Creator.php (1)
__construct(27-38)src/Cms/Services/Tag/Creator.php (1)
__construct(22-26)src/Cms/Models/Post.php (2)
__construct(49-52)setContent(142-147)
src/Cms/Services/Post/Creator.php (5)
src/Cms/Services/Category/Creator.php (1)
__construct(25-29)src/Cms/Services/Page/Creator.php (1)
__construct(23-27)src/Cms/Services/Post/Updater.php (1)
__construct(26-37)src/Cms/Services/Tag/Creator.php (1)
__construct(22-26)src/Cms/Models/Post.php (2)
__construct(49-52)setContent(142-147)
🔇 Additional comments (4)
src/Cms/Services/Tag/Creator.php (1)
19-26: Tag creator RNG injection and slug fallback look consistentThe injected random generator and the
'tag-' . uniqueId()fallback align with the other services’ pattern and keep the API ofcreate()unchanged. Nothing concerning here from a correctness or testability standpoint.Also applies to: 53-67
src/Cms/Services/Post/Updater.php (1)
44-57: Update flow correctly switches to Editor.js content handlingUsing
string $content(Editor.js JSON) and calling$post->setContent( $content )matches thePostmodel’s semantics (updatescontent_rawand derives the plain-text body), so the update path stays consistent with the new create flow and content model.Also applies to: 67-88
src/Cms/Services/Category/Creator.php (1)
22-29: Category creator random injection and slug behavior look goodThe optional random dependency and the
'category-' . uniqueId()fallback are consistent with the rest of the services and keepcreate()’s external behavior the same, aside from making slug generation testable. No issues from a correctness perspective.Also applies to: 79-93
src/Cms/Services/Category/Updater.php (1)
23-29: Updater now mirrors Creator’s slug generation patternInjecting the same random source and using the same
'category-' . uniqueId()fallback keeps create/update slug behavior aligned and improves testability. The rest of the update flow is unchanged.Also applies to: 80-93
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (32)
tests/Unit/Cms/Cli/Commands/Install/InstallCommandTest.php (1)
8-9: Namespace updates look good, but imports appear unused.The namespace change from
SettingtoSettingsis consistent with the PR's refactoring objectives. However, neitherSettingManagernorYamlappear to be referenced anywhere in this test file—onlyInstallCommandis actually used.🔎 Suggested cleanup
use Neuron\Cms\Cli\Commands\Install\InstallCommand; use org\bovigo\vfs\vfsStream; -use Neuron\Data\Settings\SettingManager; -use Neuron\Data\Settings\Source\Yaml;tests/Unit/Cms/Services/AuthenticationTest.php (1)
80-80: Consider adding test coverage for the new column.The
two_factor_recovery_codescolumn has been added to support 2FA functionality, but there's no test coverage for this field in the authentication tests. While not blocking, consider adding tests to verify that recovery codes are properly stored and retrieved.tests/Unit/Cms/Repositories/DatabaseUserRepositoryTest.php (1)
66-66: Consider adding test coverage for the new column.The
two_factor_recovery_codescolumn supports 2FA recovery functionality but lacks test coverage. Consider adding tests to verify the storage and retrieval of recovery codes.tests/Unit/Cms/Repositories/DatabasePostRepositoryTest.php (2)
56-76: Consider whether the users table is necessary.A full
userstable is created in the test setup but never populated or directly used. The tests reference author IDs (1, 2) without creating corresponding user records. While this works for testing post repository operations in isolation, consider whether this table adds unnecessary complexity to the test setup.
85-85: Consider adding test coverage for the new content_raw column.The
content_rawcolumn has been added with a JSON default value to support Editor.js content, but there's no test coverage for this field. Consider adding tests to verify that Editor.js content is properly stored and retrieved in this format.tests/Unit/Cms/Services/Email/SenderTest.php (5)
19-19: Consider more robust path construction.The relative path traversal (
__DIR__ . '/../../../../..') is fragile and will break if the test file is moved. Consider using a constant defined at the project root ordirname(__FILE__, 6)for clarity.
26-93: Consider verifying internal state, not just return values.The fluent interface tests only assert that methods return
$this, but they don't verify that the data (email addresses, subject, body, attachments) is actually stored correctly. While testing the fluent interface is valuable, these tests could pass even if the methods silently fail to store data.Consider adding assertions that verify the data was stored, or add integration tests that check the final state before
send()is called.
95-125: Verify the rendered template content.The test creates a template with
<h1>Hello <?= $name ?></h1>and passes['name' => 'John'], but it doesn't verify that the body was actually set to the expected rendered output. Consider adding an assertion that checks the rendered content matches the expected result.🔎 Suggested verification
After calling
template(), you could use reflection to verify the private_bodyproperty was set correctly, or refactor the Sender class to expose a getter for testing purposes:$result = $this->sender->template( 'email/test-email', [ 'name' => 'John' ] ); $this->assertSame( $this->sender, $result, 'Should return self for fluent interface' ); + + // Use reflection to verify the rendered content + $reflection = new \ReflectionClass( $this->sender ); + $bodyProperty = $reflection->getProperty( '_body' ); + $bodyProperty->setAccessible( true ); + $body = $bodyProperty->getValue( $this->sender ); + + $this->assertStringContainsString( '<h1>Hello John</h1>', $body, 'Template should be rendered with data' );
120-120: Fragile directory cleanup logic.The condition
count(scandir($templateDir)) === 2assumes the directory only contains.and.., but this may not hold if other files exist or if the test fails partway through. Consider using a more robust cleanup approach or a dedicated temp directory that can be safely removed regardless of contents.
135-176: Consider verifying that logging actually occurs.Both test mode tests (
testSendInTestModeLogsEmailandtestSendWithMultipleRecipients) only assert thatsend()returnstrue, but they don't verify that the email data was actually logged as expected. Consider adding assertions or using a log spy/mock to verify thatLog::info()was called with the expected messages.tests/Unit/Cms/Services/Media/MediaValidatorTest.php (2)
36-58: Minor inconsistency in assertion style.Line 44 uses
assertEqualsfor the exact error message "No file was uploaded", while line 57 usesassertStringContainsStringfor what appears to be the same error message. Consider using consistent assertion methods for the same error condition.🔎 Suggested fix for consistency
- $this->assertStringContainsString( 'No file was uploaded', $this->_validator->getFirstError() ); + $this->assertEquals( 'No file was uploaded', $this->_validator->getFirstError() );
36-161: Consider adding tests for successful validation and error state clearing.The test suite comprehensively covers error cases but lacks:
- A happy path test where validation succeeds (though the comment at lines 163-168 acknowledges the complexity of creating valid image data in unit tests)
- A test verifying that errors are cleared between successive
validate()callsWhile these additions are optional given the challenges with unit testing image validation, they would strengthen the test coverage.
tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php (2)
31-41: Consider testing individual missing configuration values.The current test verifies behavior when all Cloudinary configuration is missing. Based on the implementation, the constructor should also throw when any individual value (cloud_name, api_key, or api_secret) is missing. Adding separate tests for each missing value would provide more granular coverage.
68-113: Good approach to marking integration tests as incomplete.The incomplete tests appropriately acknowledge that they require real Cloudinary credentials and network access. The example code comments provide helpful guidance for future integration testing.
One consideration: these integration tests are in the
tests/Unit/directory. You might want to move them to a separatetests/Integration/directory when implementing them, to maintain clearer separation between unit and integration tests.tests/Unit/Cms/Repositories/DatabasePageRepositoryTest.php (2)
114-130: LGTM! Good coverage of find-by-ID scenarios.Both happy path and non-existent record cases are tested appropriately.
Optional: Verify eager loading of author relationship
The repository implementation uses
Page::with('author')for eager loading. You could optionally add an assertion to verify the author is loaded:public function testFindByIdReturnsPage(): void { $page = $this->createTestPage(); $found = $this->repository->findById( $page->getId() ); $this->assertNotNull( $found ); $this->assertEquals( $page->getId(), $found->getId() ); $this->assertEquals( $page->getTitle(), $found->getTitle() ); + $this->assertNotNull( $found->getAuthor() ); // Verify eager loading }
250-284: LGTM! Good coverage of retrieval and filtering.The tests properly verify basic retrieval, status filtering, and pagination with limit/offset.
Optional: Verify DESC ordering by created_at
The repository orders results by
created_at DESC. You could optionally verify this ordering:public function testAllOrdersByCreatedAtDesc(): void { // Create pages with slight delays or explicit timestamps $page1 = $this->createTestPage(['title' => 'First']); $page2 = $this->createTestPage(['title' => 'Second']); $page3 = $this->createTestPage(['title' => 'Third']); $pages = $this->repository->all(); $this->assertEquals('Third', $pages[0]->getTitle()); $this->assertEquals('Second', $pages[1]->getTitle()); $this->assertEquals('First', $pages[2]->getTitle()); }tests/Unit/Cms/Services/Widget/WidgetRendererTest.php (1)
79-90: Consider asserting the render result.The test correctly verifies that the custom limit is passed to the repository. For completeness, consider adding an assertion on the render result to ensure the method executes successfully.
🔎 Optional enhancement
$renderer = new WidgetRenderer( $repository ); - $renderer->render( 'latest-posts', ['limit' => 10] ); + $result = $renderer->render( 'latest-posts', ['limit' => 10] ); + + $this->assertStringContainsString( 'latest-posts-widget', $result );tests/TESTING.md (2)
7-7: Consider adding a language identifier to the fenced code block.The fenced code block showing the directory structure could include a language identifier (e.g.,
textorplaintext) for better markdown rendering consistency.🔎 Proposed fix
-``` +```text tests/ ├── Unit/ # Unit tests (fast, isolated, use mocks)
237-237: Capitalize "GitHub" for consistency.The official platform name should be capitalized as "GitHub" rather than "github".
🔎 Proposed fix
-The CI workflow (`.github/workflows/ci.yml`) runs **both unit and integration tests** to ensure comprehensive validation. +The CI workflow (`.github/workflows/ci.yml`) runs **both unit and integration tests** to ensure comprehensive validation.Note: The path
.github/workflows/ci.ymlshould remain lowercase as it's a file path reference.tests/Unit/Cms/Services/Content/EditorJsRendererTest.php (1)
153-194: Consider adding tests for image styling options.The
EditorJsRenderer::renderImage()supportsstretched,withBorder, andwithBackgroundoptions that add CSS classes. Current tests only verify basic image rendering. Consider adding a test case that validates these styling options if they're important for the feature.🔎 Example test case for image styling options
public function testRenderImageWithStylingOptions(): void { $data = [ 'blocks' => [ [ 'type' => 'image', 'data' => [ 'file' => [ 'url' => 'https://example.com/image.jpg' ], 'stretched' => true, 'withBorder' => true, 'withBackground' => true ] ] ] ]; $result = $this->renderer->render( $data ); $this->assertStringContainsString( 'w-100', $result ); // stretched $this->assertStringContainsString( 'border', $result ); // withBorder $this->assertStringContainsString( 'bg-light', $result ); // withBackground }tests/Unit/Cms/Services/Page/DeleterTest.php (1)
10-66: Good test coverage for Page Deleter.Tests cover the essential scenarios: successful deletion, handling pages without IDs, and repository failures. The test structure is clean and follows project conventions.
Note: Unlike Post\Deleter which includes a
deleteById()method, Page\Deleter only provides adelete()method that accepts a Page entity. The test suite appropriately covers all available methods.tests/Unit/Cms/Services/Page/UpdaterTest.php (1)
60-63: Consider adding->with($page)constraint for consistency.While this mock setup works correctly, it's less strict than the first test method. For consistency and better verification across all tests, consider adding
->with($page)to the mock expectation.🔎 Suggested improvement
$repository ->expects( $this->once() ) ->method( 'update' ) + ->with( $page ) ->willReturn( true );tests/Integration/CategoryTagRelationshipTest.php (1)
25-50: Remove unused variables or utilize them to test parent-child relationships.The static analysis correctly flags
$parentId(line 36) and$childId(line 40) as unused. The docblock mentions "parent category" and "child category", but the code doesn't actually establish or test a parent-child hierarchy—both categories are created as standalone entries.Either remove these variables if hierarchy testing isn't needed, or add assertions that verify parent-child relationships if that's the intended test scope.
🔎 Proposed fix to remove unused variables
$stmt->execute( ['Technology', 'technology', 'Tech articles', $now, $now] ); - $parentId = (int)$this->pdo->lastInsertId(); + $this->pdo->lastInsertId(); // Parent category created // Create child category $stmt->execute( ['PHP', 'php', 'PHP programming', $now, $now] ); - $childId = (int)$this->pdo->lastInsertId(); + $this->pdo->lastInsertId(); // Child category createdtests/Integration/IntegrationTestCase.php (3)
44-47: Remove emptysetUpBeforeClassoverride.This method only calls
parent::setUpBeforeClass()with no additional logic. It can be safely removed as PHPUnit will call the parent automatically.🔎 Proposed fix
- /** - * Set up test database before each test class - */ - public static function setUpBeforeClass(): void - { - parent::setUpBeforeClass(); - } -
272-284: Consider validating table name to prevent SQL injection.While this is a protected test helper unlikely to receive untrusted input, the raw string interpolation for the table name is a risky pattern. Consider adding a simple validation to ensure the table name matches expected characters.
🔎 Proposed fix with table name validation
protected function truncateTable( string $table ): void { + // Validate table name contains only safe characters + if( !preg_match( '/^[a-zA-Z_][a-zA-Z0-9_]*$/', $table ) ) + { + throw new \InvalidArgumentException( "Invalid table name: {$table}" ); + } + $driver = $this->pdo->getAttribute( PDO::ATTR_DRIVER_NAME ); if( $driver === 'sqlite' )
232-238: Make$migrationManagernullable for proper null safety.The property is declared as
Managerwithout?, butrollbackMigrationschecks if it's truthy. In PHP 8+, accessing an uninitialized typed property throws anError. Either make the type nullable (?Manager) or ensure it's always initialized.🔎 Proposed fix
protected PDO $pdo; - protected Manager $migrationManager; + protected ?Manager $migrationManager = null; private static bool $migrationsRun = false;tests/Integration/UserAuthenticationFlowTest.php (1)
259-324: Consider testing the actual lockout enforcement.The test simulates failed login attempts and account locking, but doesn't verify that a locked account actually prevents login. Consider adding an assertion that queries for the lock status before the successful login reset, to ensure the lock mechanism would work as expected.
Current test verifies data persistence but not the business logic enforcement.
tests/Integration/PageManagementFlowTest.php (2)
109-111: Consider removingsleep(1)in favor of a deterministic approach.Using
sleep(1)to ensure timestamp differences slows down the test suite. Since you're already capturing$nowbefore the insert and$newNowafter the sleep, you could instead rely on comparing the actual stored values or manually set different timestamps.🔎 Proposed fix
$pageId = (int)$this->pdo->lastInsertId(); // Update page - sleep(1); // Ensure updated_at is different + // Use a slightly different timestamp for the update $updatedContent = json_encode([ 'blocks' => [ ['type' => 'paragraph', 'data' => ['text' => 'Updated content']] ] ]); $stmt = $this->pdo->prepare( "UPDATE pages SET title = ?, content = ?, template = ?, meta_title = ?, updated_at = ? WHERE id = ?" ); - $newNow = date( 'Y-m-d H:i:s' ); + // Add one second to ensure the timestamp is different + $newNow = date( 'Y-m-d H:i:s', strtotime( $now ) + 1 ); $stmt->execute([
331-337: Consider using prepared statement outside the loop for better efficiency.The
$stmtprepared statement inside the loop on lines 334-336 is re-prepared on each iteration. While this works correctly, preparing it once before the loop would be more efficient.🔎 Proposed refactor
// Simulate 5 page views + $updateStmt = $this->pdo->prepare( + "UPDATE pages SET view_count = view_count + 1 WHERE id = ?" + ); + $selectStmt = $this->pdo->prepare( "SELECT view_count FROM pages WHERE id = ?" ); + for( $i = 1; $i <= 5; $i++ ) { - $stmt = $this->pdo->prepare( - "UPDATE pages SET view_count = view_count + 1 WHERE id = ?" - ); - $stmt->execute( [$pageId] ); + $updateStmt->execute( [$pageId] ); // Verify count incremented - $stmt = $this->pdo->prepare( "SELECT view_count FROM pages WHERE id = ?" ); - $stmt->execute( [$pageId] ); - $count = (int)$stmt->fetchColumn(); + $selectStmt->execute( [$pageId] ); + $count = (int)$selectStmt->fetchColumn(); $this->assertEquals( $i, $count ); }tests/Integration/TagManagementTest.php (2)
218-226: Remove unused variables$unusedTagId1and$unusedTagId2.These variables are captured but never used in assertions. The test already verifies unused tags by name, so the IDs are unnecessary. Based on static analysis hints.
🔎 Proposed fix
$stmt->execute( ['used-tag', 'used-tag', $now, $now] ); $usedTagId = (int)$this->pdo->lastInsertId(); - $stmt->execute( ['unused-tag-1', 'unused-tag-1', $now, $now] ); - $unusedTagId1 = (int)$this->pdo->lastInsertId(); - - $stmt->execute( ['unused-tag-2', 'unused-tag-2', $now, $now] ); - $unusedTagId2 = (int)$this->pdo->lastInsertId(); + $stmt->execute( ['unused-tag-1', 'unused-tag-1', $now, $now] ); + $stmt->execute( ['unused-tag-2', 'unused-tag-2', $now, $now] );
385-405: Potential test flakiness if tag names have same post_count.The query orders by
post_count DESCbut doesn't include a secondary sort. If two tags have the same count, the order could be non-deterministic. In this specific test the counts are unique (15, 10, 5, 2), so it's not an issue now, but adding a secondary sort would make the test more robust.🔎 Proposed fix for deterministic ordering
$stmt = $this->pdo->prepare( "SELECT t.name, COUNT(pt.post_id) as post_count FROM tags t LEFT JOIN post_tags pt ON t.id = pt.tag_id GROUP BY t.id, t.name - ORDER BY post_count DESC + ORDER BY post_count DESC, t.name ASC LIMIT 3" );tests/Unit/Cms/Services/CsrfTokenTest.php (1)
179-186: Optional: Regex could be more specific for hex format.The test correctly validates that tokens are URL-safe alphanumeric characters. However, since the implementation uses
'hex'format (which produces[0-9a-f]characters), the regex could be more precise to match the actual format:- $this->assertMatchesRegularExpression('/^[a-zA-Z0-9]+$/', $token); + $this->assertMatchesRegularExpression('/^[0-9a-f]+$/', $token);This is a minor refinement and the current test will pass correctly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
.github/workflows/ci.yml(1 hunks)UPGRADE_NOTES.md(1 hunks)resources/database/migrate/20250111000000_create_users_table.php(0 hunks)src/Cms/Auth/Filters/CsrfFilter.php(1 hunks)src/Cms/Controllers/Traits/UsesDtos.php(2 hunks)tests/Integration/CategoryTagRelationshipTest.php(1 hunks)tests/Integration/IntegrationTestCase.php(1 hunks)tests/Integration/PageManagementFlowTest.php(1 hunks)tests/Integration/PostPublishingFlowTest.php(1 hunks)tests/Integration/PostPublishingSchedulingTest.php(1 hunks)tests/Integration/TagManagementTest.php(1 hunks)tests/Integration/UserAuthenticationFlowTest.php(1 hunks)tests/TESTING.md(1 hunks)tests/Unit/Cms/BlogControllerTest.php(3 hunks)tests/Unit/Cms/Cli/Commands/Install/InstallCommandTest.php(1 hunks)tests/Unit/Cms/ContentControllerTest.php(1 hunks)tests/Unit/Cms/Maintenance/MaintenanceConfigTest.php(1 hunks)tests/Unit/Cms/Repositories/DatabaseCategoryRepositoryTest.php(2 hunks)tests/Unit/Cms/Repositories/DatabaseEmailVerificationTokenRepositoryTest.php(1 hunks)tests/Unit/Cms/Repositories/DatabasePageRepositoryTest.php(1 hunks)tests/Unit/Cms/Repositories/DatabasePasswordResetTokenRepositoryTest.php(1 hunks)tests/Unit/Cms/Repositories/DatabasePostRepositoryTest.php(3 hunks)tests/Unit/Cms/Repositories/DatabaseTagRepositoryTest.php(2 hunks)tests/Unit/Cms/Repositories/DatabaseUserRepositoryTest.php(3 hunks)tests/Unit/Cms/Services/AuthenticationTest.php(3 hunks)tests/Unit/Cms/Services/Content/EditorJsRendererTest.php(1 hunks)tests/Unit/Cms/Services/Content/ShortcodeParserTest.php(1 hunks)tests/Unit/Cms/Services/CsrfTokenTest.php(4 hunks)tests/Unit/Cms/Services/Dto/DtoFactoryServiceTest.php(1 hunks)tests/Unit/Cms/Services/Email/SenderTest.php(1 hunks)tests/Unit/Cms/Services/EmailVerifierTest.php(2 hunks)tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php(1 hunks)tests/Unit/Cms/Services/Media/MediaValidatorTest.php(1 hunks)tests/Unit/Cms/Services/Page/CreatorTest.php(1 hunks)tests/Unit/Cms/Services/Page/DeleterTest.php(1 hunks)tests/Unit/Cms/Services/Page/UpdaterTest.php(1 hunks)tests/Unit/Cms/Services/PasswordResetterTest.php(2 hunks)tests/Unit/Cms/Services/Post/CreatorTest.php(9 hunks)tests/Unit/Cms/Services/Post/UpdaterTest.php(10 hunks)tests/Unit/Cms/Services/RegistrationServiceTest.php(1 hunks)tests/Unit/Cms/Services/Widget/WidgetRegistryTest.php(1 hunks)tests/Unit/Cms/Services/Widget/WidgetRendererTest.php(1 hunks)tests/Unit/Cms/Services/Widget/WidgetTest.php(1 hunks)tests/phpunit.xml(1 hunks)
💤 Files with no reviewable changes (1)
- resources/database/migrate/20250111000000_create_users_table.php
✅ Files skipped from review due to trivial changes (1)
- tests/Unit/Cms/Services/Content/ShortcodeParserTest.php
🧰 Additional context used
🧬 Code graph analysis (14)
tests/Unit/Cms/Services/Page/DeleterTest.php (3)
tests/Unit/Cms/Services/Category/DeleterTest.php (1)
DeleterTest(10-83)tests/Unit/Cms/Services/Post/DeleterTest.php (1)
DeleterTest(10-92)tests/Unit/Cms/Services/User/DeleterTest.php (1)
DeleterTest(10-83)
tests/Unit/Cms/Services/Post/UpdaterTest.php (1)
src/Cms/Models/Post.php (1)
getBody(108-111)
tests/Unit/Cms/Services/Media/MediaValidatorTest.php (1)
src/Cms/Services/Media/MediaValidator.php (3)
MediaValidator(14-192)getErrors(178-181)getFirstError(188-191)
tests/Unit/Cms/Services/CsrfTokenTest.php (1)
src/Cms/Services/Auth/CsrfToken.php (2)
CsrfToken(17-75)generate(32-37)
tests/Unit/Cms/Services/Widget/WidgetRegistryTest.php (2)
src/Cms/Services/Widget/WidgetRegistry.php (2)
WidgetRegistry(15-87)getAll(61-64)src/Cms/Services/Content/ShortcodeParser.php (1)
ShortcodeParser(15-178)
tests/Integration/CategoryTagRelationshipTest.php (1)
tests/Integration/IntegrationTestCase.php (2)
IntegrationTestCase(35-285)createTestUser(243-267)
tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php (1)
src/Cms/Services/Media/CloudinaryUploader.php (1)
CloudinaryUploader(16-197)
tests/Integration/PostPublishingFlowTest.php (1)
tests/Integration/IntegrationTestCase.php (2)
IntegrationTestCase(35-285)createTestUser(243-267)
tests/Unit/Cms/Repositories/DatabasePageRepositoryTest.php (1)
src/Cms/Repositories/DatabasePageRepository.php (1)
DatabasePageRepository(16-181)
tests/Unit/Cms/Services/Content/EditorJsRendererTest.php (2)
src/Cms/Services/Content/EditorJsRenderer.php (1)
EditorJsRenderer(12-241)src/Cms/Services/Content/ShortcodeParser.php (1)
ShortcodeParser(15-178)
tests/Integration/UserAuthenticationFlowTest.php (1)
tests/Integration/IntegrationTestCase.php (2)
IntegrationTestCase(35-285)createTestUser(243-267)
tests/Integration/TagManagementTest.php (1)
tests/Integration/IntegrationTestCase.php (2)
IntegrationTestCase(35-285)createTestUser(243-267)
tests/Unit/Cms/Repositories/DatabasePasswordResetTokenRepositoryTest.php (2)
src/Cms/Models/PasswordResetToken.php (1)
PasswordResetToken(14-178)src/Cms/Repositories/DatabasePasswordResetTokenRepository.php (1)
DatabasePasswordResetTokenRepository(19-117)
tests/Unit/Cms/Services/Email/SenderTest.php (1)
src/Cms/Services/Email/Sender.php (10)
Sender(17-271)to(40-44)cc(49-53)bcc(58-62)subject(67-71)body(76-81)replyTo(86-91)attach(96-100)template(105-132)send(137-197)
🪛 LanguageTool
UPGRADE_NOTES.md
[style] ~44-~44: The double modal “Required Run” is nonstandard (only accepted in certain dialects). Consider “to be Run”.
Context: ... Changes - None ### Action Required 1. Run php neuron cms:upgrade to copy new mi...
(NEEDS_FIXED)
tests/TESTING.md
[uncategorized] ~237-~237: The official name of this software platform is spelled with a capital “H”.
Context: ...tion (GitHub Actions) The CI workflow (.github/workflows/ci.yml) runs **both unit and...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
tests/TESTING.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 PHPMD (2.15.0)
tests/Unit/Cms/Services/Widget/WidgetRegistryTest.php
186-186: Avoid unused parameters such as '$name'. (undefined)
(UnusedFormalParameter)
tests/Integration/CategoryTagRelationshipTest.php
36-36: Avoid unused local variables such as '$parentId'. (undefined)
(UnusedLocalVariable)
40-40: Avoid unused local variables such as '$childId'. (undefined)
(UnusedLocalVariable)
tests/Unit/Cms/Services/Widget/WidgetTest.php
16-16: Avoid unused parameters such as '$attrs'. (undefined)
(UnusedFormalParameter)
tests/Unit/Cms/Repositories/DatabasePageRepositoryTest.php
218-218: Avoid unused local variables such as '$page1'. (undefined)
(UnusedLocalVariable)
src/Cms/Controllers/Traits/UsesDtos.php
53-53: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
tests/Integration/TagManagementTest.php
222-222: Avoid unused local variables such as '$unusedTagId1'. (undefined)
(UnusedLocalVariable)
225-225: Avoid unused local variables such as '$unusedTagId2'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (89)
tests/Unit/Cms/ContentControllerTest.php (1)
6-6: LGTM! Namespace update aligns with project-wide refactoring.The import path has been correctly updated from
Neuron\Data\Setting\Source\MemorytoNeuron\Data\Settings\Source\Memory, reflecting the namespace reorganization from singular to plural. The change is consistent with the broader namespace realignment mentioned in the PR objectives.tests/Unit/Cms/Maintenance/MaintenanceConfigTest.php (1)
6-6:andUPGRADE_NOTES.md (4)
149-149: Verify/CLAUDE.mdis a legitimate documentation file.Line 149 references
/CLAUDE.mdas a help resource. This filename is unconventional and may be a debug artifact or internal documentation. Clarify whether this file:
- Is intended for end users
- Should be included in the upgrade guidance
- Should be renamed or removed
50-50: VerifyMIGRATIONS.mdexists and is accessible.The document references
MIGRATIONS.mdin lines 50, 149, and 151 as the migration best practices guide. Ensure this file:
- Exists in the repository
- Is properly documented
- Is linked or discoverable from the upgrade process
Also applies to: 149-149, 151-151
140-143: Verify version dates align with release timeline.The Version History table shows:
- Version 2025.12.5 with release date 2025-12-05
- Current date is 2025-12-19
The version 2025.12.5 release date is in the past relative to the PR creation date (2025-11-29), which is inconsistent. Verify that version dates accurately reflect the intended or actual release dates.
96-96: Verify ifcms:upgrade --checkflag is actually implemented in UpgradeCommand.Line 96 references a
--checkflag forcms:upgrade, and line 113 references a--fakeflag for database migrations. While Neuron CMS uses robmorgan/phinx (^0.16) for database migrations, which supports the--fakeflag as a standard feature, the existence and implementation of the--checkflag in UpgradeCommand requires direct code inspection to confirm. Verify in the UpgradeCommand class that these flags are properly defined and documented.tests/phpunit.xml (1)
10-13: LGTM! Test migration complete and properly organized.The separation of unit and integration tests into distinct directories follows best practices. All 57 unit tests and 7 integration tests have been successfully migrated with no orphaned tests remaining, and PHPUnit is correctly configured to discover tests in both directories.
tests/Unit/Cms/Services/RegistrationServiceTest.php (1)
10-11: Namespace migration is correct and verified complete.The update from
Neuron\Data\Setting\...toNeuron\Data\Settings\...on lines 10-11 is part of a successful namespace reorganization across the entire codebase. Verification confirms no lingering references to the oldSettingnamespace exist, and the newSettingsnamespace is consistently used in 25+ production files including services, repositories, controllers, and initializers. The test file imports align with the production code, includingRegistrationService.phpwhich uses the same new namespaces.tests/Unit/Cms/Repositories/DatabaseCategoryRepositoryTest.php (1)
8-8: LGTM! ORM integration is correctly implemented.The addition of ORM initialization via
Model::setPdo()properly configures the test environment to work with ORM-backed repository operations. The initialization is correctly placed before repository construction.Also applies to: 33-34
tests/Unit/Cms/Repositories/DatabaseEmailVerificationTokenRepositoryTest.php (1)
8-8: LGTM! Namespace update aligns with project refactoring.The import path has been correctly updated from
Neuron\Data\Setting\SettingManagertoNeuron\Data\Settings\SettingManager, consistent with the namespace reorganization across the PR.tests/Unit/Cms/Services/AuthenticationTest.php (1)
11-12: LGTM! ORM and namespace updates are correctly implemented.The namespace change to
Neuron\Data\Settings\SettingManagerand ORM initialization viaModel::setPdo()are properly configured for the test environment.Also applies to: 50-51
tests/Unit/Cms/Repositories/DatabaseUserRepositoryTest.php (1)
8-9: LGTM! ORM integration is correctly configured.The namespace update and ORM initialization are properly implemented, following the consistent pattern across the test suite.
Also applies to: 47-48
tests/Unit/Cms/Repositories/DatabasePasswordResetTokenRepositoryTest.php (1)
1-253: LGTM! Comprehensive test coverage for password reset tokens.This new test file provides excellent coverage of the
DatabasePasswordResetTokenRepository:
- Creation and persistence of tokens
- Token retrieval and expiration handling
- Deletion operations (by email, by token, expired tokens)
- Edge cases (nonexistent tokens, multiple tokens per email)
The test structure is clear with good use of helper methods and proper assertions.
tests/Unit/Cms/Repositories/DatabaseTagRepositoryTest.php (1)
8-8: LGTM! ORM integration follows established pattern.The ORM initialization is correctly implemented and consistent with other repository tests in this PR.
Also applies to: 33-34
tests/Unit/Cms/Repositories/DatabasePostRepositoryTest.php (1)
10-10: LGTM! ORM initialization is correctly configured.The addition of ORM support follows the consistent pattern established across the test suite.
Also applies to: 35-36
tests/Unit/Cms/Services/Email/SenderTest.php (1)
178-192: Constructor tests look good.The constructor variation tests appropriately verify that the
Senderclass can be instantiated with different parameter combinations without throwing exceptions. This is a reasonable level of coverage for constructor flexibility.tests/Unit/Cms/Services/PasswordResetterTest.php (2)
43-43: Path correction appears accurate for the current file location.The update from 3 to 4 directory levels correctly reflects the file's depth within the project structure (
tests/Unit/Cms/Services/). The$_basePathis used byPasswordResetterfor resource access (line 52), so verify that templates and other resources load correctly with this path.
11-12: Verify the namespace changes against the library version.The namespace updates from
SettingtoSettingsrequire confirmation against neuron-php/data 0.9.0 source code. Web documentation shows conflicting references to both singular and plural forms of the namespace, and the test directory structure referencesSetting/(singular). Ensure the imports match the actual implementation in the specific library version being used.src/Cms/Auth/Filters/CsrfFilter.php (1)
68-73: Good security improvement using filtered input.The refactoring from direct
$_POSTaccess toPost::filterScalar('csrf_token')enhances security by applying input filtering. The truthy check on line 70 will work correctly: whencsrf_tokenis missing from POST data, filter_input() returns null, which is falsy in PHP, allowing the header fallback at lines 75-79 to execute as intended. The return type compatibility is sound—filterScalar returnsmixed, which includes null values acceptable for the conditional flow.tests/Unit/Cms/Services/Dto/DtoFactoryServiceTest.php (1)
17-17: Path is correct and resolves properly.The path adjustment successfully points to
src/Cms/Dtos. The directory exists and contains all expected DTO files (CreateCategoryDto, CreatePostDto, CreateUserDto, MediaUploadDto, RegisterUserDto, UpdateCategoryDto, UpdatePostDto, UpdateUserDto).tests/Unit/Cms/Services/Media/MediaValidatorTest.php (1)
20-34: Test setup is well-structured.The virtual filesystem and in-memory settings configuration properly isolate the tests and align with the MediaValidator's expected dependencies.
tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php (1)
17-29: Test setup is appropriate.The in-memory settings configuration correctly provides all required Cloudinary credentials for testing.
tests/Unit/Cms/BlogControllerTest.php (3)
13-14: LGTM!The namespace updates from
SettingtoSettingsare consistent with the broader namespace reorganization mentioned in the PR summary.
16-16: LGTM!The ORM Model import is necessary for the ORM initialization on line 55 and aligns with the PR's migration to the Neuron ORM.
129-129: Thecontent_rawdefault value is correct.EditorJS automatically creates a default block during initialization when the blocks array is empty, so the
'{"blocks":[]}'default is appropriate for this column and matches EditorJS's expected JSON format.tests/Unit/Cms/Repositories/DatabasePageRepositoryTest.php (12)
14-18: LGTM! Clean test class structure.The class properties are well-defined for testing with an in-memory database and repository instance.
19-41: LGTM! Proper test environment setup.The setUp method correctly initializes an in-memory SQLite database, configures the ORM, and creates the repository instance. The use of Model::setPdo() aligns with the ORM-based approach mentioned in the repository implementation.
43-87: LGTM! Comprehensive schema definition.The table schemas correctly define all necessary fields for users and pages, including the foreign key constraint for referential integrity.
89-112: LGTM! Well-designed test helpers.The helper methods effectively reduce duplication and use
uniqid()to ensure unique test data. The EditorJS JSON format in the content field aligns with the PR's block-based editor upgrade.
132-148: LGTM! Proper coverage of slug lookup.Both successful lookup and non-existent slug scenarios are correctly tested.
150-189: LGTM! Comprehensive create operation tests.Both successful creation and duplicate slug prevention are properly tested with appropriate assertions and exception expectations.
191-228: LGTM! Thorough update operation coverage.All three update scenarios are properly tested:
- Successful update
- Update without ID
- Duplicate slug prevention
Note: The static analysis hint about
$page1being unused on line 218 is a false positive. The variable is intentionally created to occupy the 'page-one' slug, which is necessary to test duplicate slug detection.
230-248: LGTM! Complete delete operation testing.Both successful deletion and non-existent record scenarios are correctly verified.
286-314: LGTM! Proper testing of status-specific retrieval methods.Both
getPublished()andgetDrafts()are correctly tested with verification of both count and status values.
316-345: LGTM! Comprehensive author filtering tests.Both basic author filtering and combined author+status filtering are properly verified.
347-369: LGTM! Complete count operation testing.Both total count and status-filtered count scenarios are properly tested.
371-388: LGTM! Proper view count increment testing.Both successful increment with persistence and non-existent record handling are correctly verified.
tests/Unit/Cms/Services/Widget/WidgetRendererTest.php (2)
12-42: Excellent coverage of error and edge cases.The first three test methods properly validate defensive behavior: unknown widgets, missing dependencies, and empty states. Mock expectations and assertions are well-structured.
134-153: Strong XSS protection validation.The test properly verifies that potentially malicious HTML is escaped, preventing script injection vulnerabilities. This is critical for user-generated content rendering.
tests/Unit/Cms/Services/Widget/WidgetTest.php (2)
93-119: Proper temporary file cleanup in tests.Both view rendering tests correctly create temporary files and clean them up with
unlink(), preventing test pollution. This demonstrates good test hygiene.
121-179: Comprehensive HTML sanitization test coverage.The test suite thoroughly validates the sanitization allowlist, covering safe tags (div, p, strong), dangerous tags (script, iframe), and various content elements (links, images, headings, lists). The inline comment at line 137-138 helpfully documents strip_tags behavior.
tests/Unit/Cms/Services/Widget/WidgetRegistryTest.php (2)
12-126: Thorough coverage of registry lifecycle operations.The test suite comprehensively validates widget registration, unregistration, retrieval, and edge cases. Mock expectations properly verify ShortcodeParser integration, ensuring both the registry and parser stay synchronized.
172-197: Elegant callback capture pattern for integration testing.This test uses an advanced technique to capture the callback registered with ShortcodeParser and verify it correctly invokes the widget's render method. This validates the end-to-end integration between WidgetRegistry and widget rendering.
tests/TESTING.md (1)
1-300: Excellent testing documentation!This comprehensive testing guide provides clear structure, practical examples, and helpful troubleshooting tips. The organization by test type (unit vs. integration), detailed coverage breakdown, and CI integration notes make this a valuable resource for contributors.
.github/workflows/ci.yml (4)
29-36: Clarify thefail_ci_if_error: truechange.Setting
fail_ci_if_error: truemeans the CI pipeline will fail if Codecov upload encounters any errors (network issues, authentication problems, etc.). This is stricter than the previous configuration and could cause CI failures unrelated to code quality.Consider whether this strict setting is appropriate for your workflow. If Codecov availability is critical, keep it
true. If you want CI to succeed even with coverage upload failures, set it tofalse.Current setting causes CI to fail on Codecov errors. Is this the intended behavior?
1-36: Well-structured CI workflow with comprehensive test coverage.The separation of unit and integration test suites with dedicated coverage reporting provides good visibility into code quality. The use of pcov for coverage and the structured approach to testing aligns well with the testing guide documentation.
13-14: No action needed. The workflow correctly usesshivammathur/setup-php@v2, which is the recommended rolling tag that automatically receives all bug fixes, security patches, and new feature releases.
13-18: Remove the reference to Phinx — it is not a project dependency.PHP 8.4 was released in November 2024, and while testing with this version in CI is reasonable, ensure that actual dependencies (PHPUnit 9.*, Neuron ORM, PHPMailer, Cloudinary PHP) are compatible. Note that composer.json does not specify an explicit PHP version constraint, which may complicate dependency resolution across environments.
Likely an incorrect or invalid review comment.
tests/Unit/Cms/Services/EmailVerifierTest.php (2)
40-40: Base path correction is accurate and properly fixes a bug.The base path calculation has been updated from 3 directory levels up to 4 levels up. Since the test file is located at
tests/Unit/Cms/Services/EmailVerifierTest.php, the old path would resolve to thetests/directory, while the new path correctly resolves to the project root. This is a necessary fix because EmailVerifier uses the base path to locate email templates via the Sender service, and the email templates are stored atresources/views/emails/in the project root, not in the tests directory.
10-11: Namespace refactoring toSettingsis correct for neuron-php/data 0.9.x.The project correctly requires neuron-php/data 0.9.*, and the updated imports from
Neuron\Data\Settings\Source\Memoryalign with the current API. The namespace change from singularSettingto pluralSettingsis part of the library's standardized naming convention in version 0.9.x.src/Cms/Controllers/Traits/UsesDtos.php (4)
8-8: LGTM! Clean import alias.The RequestMapper alias improves code readability throughout the trait.
15-16: LGTM! Documentation accurately reflects the new behavior.The PHPDoc clearly communicates that input filtering is now handled through RequestMapper.
43-46: Verify PHPDoc claim about RequestMapper's sanitization method.The PHPDoc states RequestMapper "applies proper input sanitization via Post::filterScalar()". RequestMapper is from the external
neuron-php/dtodependency, and this specific implementation detail cannot be verified in this codebase. Confirm the claim is accurate against the actual RequestMapper source, or remove/generalize the statement to avoid misleading documentation about an external package's internal behavior.
53-65: Verify neuron-php/dto library version 0.0. and RequestMapper.mapFiltered() method.*The implementation logic is sound with appropriate conditional handling, but the external dependency requires attention. The project uses neuron-php/dto version 0.0.*, a pre-release version. While the RequestMapper.map() method is documented, the
mapFiltered()method called on line 61 is not clearly documented in public sources. Before merging, confirm that:
- Version 0.0.* is production-ready or if a stable release should be required
- The RequestMapper.mapFiltered() method exists and functions as expected
tests/Unit/Cms/Services/Post/UpdaterTest.php (3)
34-72: Well-structured test for Editor.js content migration.The test correctly validates both
getContentRaw()(the raw JSON) andgetBody()(the extracted text content), ensuring the content transformation pipeline works correctly. The assertion callback in the mock verifies all expected fields are set before the repository update is called.
74-102: LGTM!Slug generation tests correctly use Editor.js JSON content format and validate the expected slug derivation behavior.
293-363: Good coverage forpublishedAttimestamp handling.Tests correctly validate that:
publishedAtis set when transitioning to published status- Existing
publishedAtis preserved on subsequent updatesThis ensures the original publication date is never overwritten.
tests/Unit/Cms/Services/Post/CreatorTest.php (2)
35-70: Thorough validation of Editor.js content handling.The test properly validates the content transformation by checking:
getContentRaw()matches the input JSONgetBody()contains the extracted text ("Test body content")This ensures the
Postmodel correctly parses Editor.js blocks.
72-98: LGTM!Slug generation test correctly validates that slugs are derived from titles when not explicitly provided.
tests/Unit/Cms/Services/Content/EditorJsRendererTest.php (4)
9-47: Comprehensive test suite for EditorJsRenderer.Good coverage of base cases including empty data and empty blocks arrays. The paragraph test validates the essential HTML structure.
82-105: Good edge case coverage for header level clamping.Tests validate that invalid header levels (0, 10) are properly clamped to the valid range (1-6). This matches the implementation in
EditorJsRenderer::renderHeader().
338-364: Critical XSS sanitization tests.Good coverage of security-sensitive behavior:
- Script tags are stripped (XSS prevention)
- Safe inline tags (
<strong>,<em>) are preservedThis validates the
sanitizeHtml()implementation works correctly.
366-382: ShortcodeParser integration test.The mock properly validates that when content contains a shortcode bracket
[, the parser is invoked. This tests the conditional logic inparseInlineContent().tests/Unit/Cms/Services/Page/CreatorTest.php (3)
13-52: Thorough test for Page creation with all parameters.The test validates all configurable fields including meta tags (title, description, keywords), template, and content. Good use of named parameters for clarity.
137-162: Excellent slug generation edge case coverage.Tests cover important edge cases:
- Multiple spaces → single hyphen
- Special characters stripped
- Leading/trailing whitespace trimmed
This ensures robust slug generation behavior.
164-210: Good deterministic testing for fallback slugs.Using a mocked
IRandomwith predictable return values (abc123,xyz789) ensures tests are deterministic. This properly tests the fallback behavior for non-ASCII and symbol-only titles.tests/Unit/Cms/Services/Page/UpdaterTest.php (7)
1-11: LGTM! Clean test structure.The test class is well-organized with appropriate imports and namespace. The setup follows PHPUnit best practices.
12-49: LGTM! Comprehensive test coverage.This test thoroughly validates the update operation with all parameters, correctly mocking the repository and asserting all expected state changes including the updatedAt timestamp.
77-101: LGTM! Good coverage for publishing behavior.This test correctly verifies that transitioning a page to published status sets the publishedAt timestamp. The same consistency suggestion regarding
->with($page)applies here as well.
103-127: LGTM! Important test for date preservation.This test correctly verifies that updating an already-published page preserves the original publishedAt timestamp, which is crucial for maintaining accurate publication history.
129-150: LGTM! Good validation of timestamp behavior.This test properly verifies both the presence and type of the updatedAt timestamp, ensuring immutability is maintained.
152-173: LGTM! Good error handling coverage.This test correctly verifies that the updater propagates repository failures, which is essential for proper error handling in the service layer.
1-174: Excellent test coverage overall.This test file provides comprehensive coverage of the
Updaterservice with well-structured test cases covering happy paths, edge cases, and error scenarios. The tests follow PHPUnit best practices and are clearly named.The only minor inconsistency is the use of
->with($page)mock constraint in the first test but not in others. While not critical, adding this constraint throughout would provide more rigorous verification that the correct Page object is passed to the repository.tests/Integration/CategoryTagRelationshipTest.php (1)
1-538: Well-structured integration test suite for category/tag relationships.The tests comprehensively cover:
- CRUD operations for categories and tags
- Many-to-many relationships via pivot tables
- Uniqueness constraints with proper
expectExceptionusage- Cascade delete behavior
- Combined category and tag relationships on posts
The transactional isolation from
IntegrationTestCaseensures test independence.tests/Integration/IntegrationTestCase.php (1)
35-285: Solid integration test infrastructure.The base class provides:
- Multi-database support (SQLite, MySQL, PostgreSQL)
- PID-based SQLite file paths for parallel test isolation
- One-time migration execution with per-test transaction isolation
- Useful helper methods for test data creation
Good foundation for the integration test suite.
tests/Integration/PostPublishingFlowTest.php (3)
143-203: Test overlaps withCategoryTagRelationshipTest.The
testPostWithCategoriesandtestPostWithTagsmethods here test similar functionality to methods inCategoryTagRelationshipTest. Consider whether this duplication is intentional for testing different flows, or if these could be consolidated.This is acceptable if the intent is to test relationships within the publishing flow context specifically.
270-314: Good test of foreign key cascade behavior.This test validates that the
author_idforeign key onpostsis configured withON DELETE CASCADE. If the migration is misconfigured, this test will catch it—exactly what integration tests should verify.
1-388: Comprehensive post publishing flow tests.The test suite covers the complete post lifecycle including creation, updates, publishing, deletion, relationships, constraints, and transaction isolation. Well-documented with clear step numbering in
testCompletePostPublishingFlow.tests/Integration/UserAuthenticationFlowTest.php (2)
67-203: Good token security practices demonstrated in tests.The email verification and password reset token tests correctly demonstrate:
- Storing only hashed tokens in the database (SHA-256)
- Token expiration handling
- Token cleanup after use
This matches security best practices for token management.
1-422: Thorough authentication flow integration tests.The test suite covers essential authentication scenarios:
- User registration and password verification
- Email verification token lifecycle
- Password reset flow with secure token handling
- Expired token cleanup
- Login attempt tracking and account lockout
- Username and email uniqueness constraints
- Role and status persistence
Well-structured tests that validate both data persistence and security mechanisms.
tests/Integration/PostPublishingSchedulingTest.php (2)
301-308: Realistic simulation of view count increments.While a single
UPDATE posts SET view_count = 10would be more efficient, the loop correctly simulates how view tracking would work in production—validating that the atomicview_count = view_count + 1pattern works correctly across multiple operations.
1-536: Comprehensive scheduling and publishing state tests.The test suite provides excellent coverage of:
- Status transitions (draft → published → draft)
- Scheduled post handling with future/past time comparisons
- Auto-publishing logic for due scheduled posts
- View count tracking
- Status-based and time-based queries
- Published date preservation on updates
The
testFindScheduledPostsReadyToPublishtest is particularly valuable for validating scheduled publishing cron job logic.tests/Integration/PageManagementFlowTest.php (1)
1-22: LGTM! Well-structured integration test suite.The test class provides comprehensive coverage of the page management workflow including creation, updates, publishing, and cascade deletes. The use of direct PDO operations for test data setup is appropriate for integration tests.
tests/Integration/TagManagementTest.php (3)
1-21: LGTM! Comprehensive tag management test coverage.The test class covers a wide range of tag-related scenarios including creation, resolution, usage tracking, and cascade deletes. The docblock accurately describes the test coverage.
420-424: Same pattern: unused variables$orphan1Idand$orphan2Idcould be simplified.Similar to the previous finding, if you need to verify by ID (as done on line 439), keeping them is fine. However, the assertion message would be clearer if you kept track of what you're verifying.
505-516: Consider verifying array keys afterarray_filter.After
array_filter, the array keys are preserved. If downstream code relies on sequential 0-indexed keys, you'd needarray_values. The current assertions on lines 514-515 will still pass because$tagNames[0]and$tagNames[1]retain their original keys, but this is a subtle detail worth noting.tests/Unit/Cms/Services/CsrfTokenTest.php (3)
5-5: LGTM! Good testability improvement.The addition of FakeRandom dependency injection enables deterministic testing of CSRF token generation, which improves test reliability and makes token generation verifiable.
Also applies to: 18-18
25-31: LGTM! Proper test fixture setup.The setUp() correctly initializes FakeRandom with a deterministic seed and injects it into CsrfToken, enabling consistent and repeatable test execution.
59-73: LGTM! Correctly tests token uniqueness with deterministic RNG.The test properly verifies that different seeds produce different tokens, maintaining the intent of the original test while using deterministic FakeRandom.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
UPGRADE_NOTES.md (1)
11-11: Standardize command invocation syntax throughout the document.The document inconsistently uses both
./vendor/bin/neuron(lines 11, 17, 22) andphp neuron(lines 44, 45, 82, 96-98, 107, 110, 132). This creates confusion for users following the upgrade instructions.🔎 Recommended fix
Choose one format and apply consistently. I recommend
php neuronas it's more common in PHP CLI documentation:1. **Run the upgrade command:** ```bash - ./vendor/bin/neuron cms:upgrade + php neuron cms:upgrade ``` 2. **Review and apply migrations:** The upgrade command will detect new migrations. Review them and run: ```bash - ./vendor/bin/neuron db:migrate + php neuron db:migrate ``` 3. **Clear caches:** ```bash - ./vendor/bin/neuron cache:clear # if applicable + php neuron cache:clear # if applicable ```Apply the same pattern to all command references throughout the document.
Based on past review comments flagging this same issue.
Also applies to: 44-45, 82-82, 96-98, 107-107, 110-110, 132-132
🧹 Nitpick comments (10)
src/Cms/Cli/Commands/Install/UpgradeCommand.php (1)
21-28: Consider a more maintainable approach to path detection.The nested
dirname()calls (5 levels deep) are brittle and will break if the directory structure changes. Consider using__DIR__with a known relative path or a configuration constant.🔎 Alternative approach
public function __construct() { // Get project root (where composer.json is) $this->_projectPath = getcwd(); // Get component path - $this->_componentPath = dirname( dirname( dirname( dirname( dirname( __DIR__ ) ) ) ) ); + // From src/Cms/Cli/Commands/Install/UpgradeCommand.php, go up 5 levels to package root + $this->_componentPath = realpath( __DIR__ . '/../../../../../' ); }Using
realpath()with a relative path is more explicit and easier to understand than countingdirname()calls.MIGRATIONS.md (1)
307-310: Add language specifiers to fenced code blocks.For better syntax highlighting and markdown compliance, specify the language for the fenced code blocks showing example terminal output.
🔎 Proposed fix
At line 307:
-``` +```text ✅ 20251205120000_add_two_factor_recovery_codes_to_users.php ❌ 20251205120000_update_users.phpAt line 407: ```diff -``` +```text SQLSTATE[HY000]: General error: 1 duplicate column name</details> Also applies to: 407-409 </blockquote></details> <details> <summary>src/Cms/Auth/Filters/AuthCsrfFilter.php (1)</summary><blockquote> `109-118`: **Consider using a response object instead of direct HTML output.** The `respondForbidden` method directly outputs HTML and calls `exit`, which tightly couples the filter to a specific response format. While this works and properly uses `htmlspecialchars` to prevent XSS, consider refactoring to use a response object or template system for better separation of concerns and testability. This is a minor point and can be deferred if the direct output approach is consistent with the rest of the codebase. </blockquote></details> <details> <summary>src/Cms/Repositories/DatabasePostRepository.php (1)</summary><blockquote> `386-401`: **Consider wrapping multiple INSERTs in a transaction.** The loop on lines 395-398 performs multiple INSERTs without a transaction. If one fails mid-loop, you'll have partial category attachments. Consider wrapping the foreach loop in a transaction for atomicity. <details> <summary>🔎 Proposed fix</summary> ```diff public function attachCategories( int $postId, array $categoryIds ): bool { if( empty( $categoryIds ) ) { return true; } + $this->_pdo->beginTransaction(); + try + { $stmt = $this->_pdo->prepare( "INSERT INTO post_categories (post_id, category_id, created_at) VALUES (?, ?, ?)" ); $now = ( new \DateTimeImmutable() )->format( 'Y-m-d H:i:s' ); foreach( $categoryIds as $categoryId ) { $stmt->execute( [ $postId, $categoryId, $now ] ); } + $this->_pdo->commit(); + } + catch( \Exception $e ) + { + $this->_pdo->rollBack(); + throw $e; + } return true; }Apply the same pattern to
attachTags(lines 417-432).src/Cms/Models/Post.php (2)
125-128: Consider logging JSON decode failures.While the fallback to
['blocks' => []]is safe, silently ignoring malformed JSON could make debugging difficult. Consider logging decode failures for troubleshooting.🔎 Suggested enhancement
public function getContent(): array { - return json_decode( $this->_contentRaw, true ) ?? ['blocks' => []]; + $decoded = json_decode( $this->_contentRaw, true ); + if( $decoded === null && json_last_error() !== JSON_ERROR_NONE ) + { + // Optional: log warning for debugging + // error_log( "Failed to decode content_raw: " . json_last_error_msg() ); + } + return $decoded ?? ['blocks' => []]; }
142-147: Consider validating JSON in setContent.While
setContentArrayexplicitly validates and throwsJsonExceptionon encoding failure,setContentaccepts any string without validation. Invalid JSON will fail silently ingetContent(). For consistency and early error detection, consider validating the JSON insetContent.🔎 Suggested enhancement
public function setContent( string $jsonContent ): self { + // Validate JSON + $decoded = json_decode( $jsonContent, true ); + if( $decoded === null && json_last_error() !== JSON_ERROR_NONE ) + { + throw new \JsonException( "Invalid JSON content: " . json_last_error_msg() ); + } $this->_contentRaw = $jsonContent; $this->_body = $this->extractPlainText( $jsonContent ); return $this; }src/Cms/Maintenance/MaintenanceManager.php (2)
70-81: Event emission logic is correct.The implementation correctly stores the result and emits the event only on success. Consider adding use statements at the top of the file for cleaner code instead of fully qualified class names.
📋 Optional refactor: Add use statements
Add these at the top of the file after line 5:
use DateTime; +use Neuron\Application\CrossCutting\Event; +use Neuron\Cms\Events\MaintenanceModeEnabledEvent; +use Neuron\Cms\Events\MaintenanceModeDisabledEvent;Then simplify the event emissions:
- \Neuron\Application\CrossCutting\Event::emit( new \Neuron\Cms\Events\MaintenanceModeEnabledEvent( + Event::emit( new MaintenanceModeEnabledEvent( $data['enabled_by'], $message ) );
206-209: Good defensive programming with JSON error handling.The added JSON error check prevents issues with corrupted maintenance files. Consider logging the error for better observability when debugging maintenance mode issues in production.
tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php (1)
148-157: Consider consolidating duplicate IP test.This test verifies the same IP address (169.254.169.254) as
testUploadFromUrlRejectsLinkLocalIp(lines 137-146). Since IP validation doesn't consider the URL path, these tests are functionally identical. Consider removing this test or consolidating both into a single test with a more general name.🔎 Proposed consolidation
Rename the test at lines 137-146 to be more comprehensive:
- public function testUploadFromUrlRejectsLinkLocalIp(): void + public function testUploadFromUrlRejectsLinkLocalAndCloudMetadataIp(): void { $uploader = new CloudinaryUploader( $this->_settings ); $this->expectException( \Exception::class ); $this->expectExceptionMessage( 'private or reserved IP address' ); - // 169.254.0.0/16 is link-local (APIPA) + // 169.254.0.0/16 is link-local (APIPA) and commonly used for cloud metadata services (e.g., 169.254.169.254) $uploader->uploadFromUrl( 'https://169.254.169.254/image.jpg' ); }And remove the duplicate test at lines 148-157.
tests/Unit/Cms/ContentControllerTest.php (1)
43-49: Consider refactoring to avoid real filesystem access.The test creates a real file in the parent directory, which can cause issues with parallel test execution and CI/CD environments. While the virtual filesystem (vfsStream) is already set up, it's not used for the version file because
ContentControllerloads from a hardcoded path.Consider one of these approaches:
- Make the version file path configurable in
ContentControllervia dependency injection- Mock the file loading mechanism in
ContentController- Use a test-specific configuration that points to the virtual filesystem
This would improve test isolation and eliminate filesystem side effects.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
MIGRATIONS.md(1 hunks)UPGRADE_NOTES.md(1 hunks)resources/app/Initializers/AuthInitializer.php(3 hunks)resources/config/routes.yaml(1 hunks)src/Cms/Auth/Filters/AuthCsrfFilter.php(1 hunks)src/Cms/Cli/Commands/Install/UpgradeCommand.php(1 hunks)src/Cms/Dtos/CategoryFieldsDto.yaml(1 hunks)src/Cms/Dtos/CreateCategoryDto.yaml(1 hunks)src/Cms/Dtos/CreatePostDto.yaml(1 hunks)src/Cms/Dtos/CreateUserDto.yaml(1 hunks)src/Cms/Dtos/PostFieldsDto.yaml(1 hunks)src/Cms/Dtos/RegisterUserDto.yaml(1 hunks)src/Cms/Dtos/UpdateCategoryDto.yaml(1 hunks)src/Cms/Dtos/UpdatePostDto.yaml(1 hunks)src/Cms/Dtos/UserCredentialsDto.yaml(1 hunks)src/Cms/Events/MaintenanceModeEnabledEvent.php(1 hunks)src/Cms/Maintenance/MaintenanceManager.php(2 hunks)src/Cms/Models/Post.php(5 hunks)src/Cms/Repositories/DatabasePostRepository.php(13 hunks)src/Cms/Services/Content/EditorJsRenderer.php(1 hunks)src/Cms/Services/Media/CloudinaryUploader.php(1 hunks)tests/Unit/Cms/ContentControllerTest.php(5 hunks)tests/Unit/Cms/Models/PostTest.php(1 hunks)tests/Unit/Cms/Services/Content/EditorJsRendererTest.php(1 hunks)tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/Cms/Dtos/UpdatePostDto.yaml
- src/Cms/Dtos/RegisterUserDto.yaml
- src/Cms/Dtos/UpdateCategoryDto.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- resources/app/Initializers/AuthInitializer.php
- tests/Unit/Cms/Services/Content/EditorJsRendererTest.php
🧰 Additional context used
🧬 Code graph analysis (8)
src/Cms/Auth/Filters/AuthCsrfFilter.php (2)
src/Cms/Services/Auth/Authentication.php (1)
Authentication(19-382)src/Cms/Services/Auth/CsrfToken.php (1)
CsrfToken(17-75)
src/Cms/Events/MaintenanceModeEnabledEvent.php (2)
src/Cms/Events/MaintenanceModeDisabledEvent.php (1)
getName(35-38)src/Cms/Events/UserLoginEvent.php (1)
getName(39-42)
src/Cms/Maintenance/MaintenanceManager.php (2)
src/Cms/Events/MaintenanceModeEnabledEvent.php (1)
MaintenanceModeEnabledEvent(23-40)src/Cms/Events/MaintenanceModeDisabledEvent.php (1)
MaintenanceModeDisabledEvent(24-39)
tests/Unit/Cms/Models/PostTest.php (1)
src/Cms/Models/Post.php (2)
setContent(142-147)getBody(108-111)
tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php (1)
src/Cms/Services/Media/CloudinaryUploader.php (3)
CloudinaryUploader(16-326)upload(67-88)uploadFromUrl(98-117)
src/Cms/Repositories/DatabasePostRepository.php (5)
src/Cms/Models/Post.php (6)
fromArray(488-585)setCategories(376-380)setTags(436-440)toArray(592-609)setId(65-69)incrementViewCount(321-325)src/Cms/Models/Tag.php (3)
fromArray(125-156)toArray(163-172)setId(44-48)src/Cms/Models/Category.php (3)
fromArray(143-175)toArray(182-192)setId(45-49)src/Cms/Models/PasswordResetToken.php (3)
fromArray(133-163)toArray(168-177)setId(48-52)src/Cms/Repositories/DatabasePageRepository.php (4)
create(50-65)count(154-164)all(101-118)incrementViewCount(169-180)
src/Cms/Cli/Commands/Install/UpgradeCommand.php (1)
src/Cms/Cli/Commands/Install/InstallCommand.php (1)
__construct(25-32)
src/Cms/Services/Media/CloudinaryUploader.php (2)
src/Cms/Controllers/Admin/Media.php (2)
Media(18-192)__construct(26-39)src/Cms/Services/Media/IMediaUploader.php (3)
upload(22-22)uploadFromUrl(32-32)delete(41-41)
🪛 LanguageTool
UPGRADE_NOTES.md
[style] ~44-~44: The double modal “Required Run” is nonstandard (only accepted in certain dialects). Consider “to be Run”.
Context: ... Changes - None ### Action Required 1. Run php neuron cms:upgrade to copy new mi...
(NEEDS_FIXED)
🪛 markdownlint-cli2 (0.18.1)
MIGRATIONS.md
307-307: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
407-407: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 PHPMD (2.15.0)
src/Cms/Cli/Commands/Install/UpgradeCommand.php
61-61: Avoid unused parameters such as '$parameters'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (67)
src/Cms/Cli/Commands/Install/UpgradeCommand.php (2)
174-178: Good fix from previous review!The use of
json_last_error()correctly addresses the previous concern about reliable JSON parse error detection.
407-427: Good fix from previous review!The unused
$installedMigrationsvariable has been correctly removed, and the code now directly uses$packageMigrations.MIGRATIONS.md (1)
1-464: Excellent comprehensive migration guide!This document provides thorough coverage of migration principles, workflows, and best practices. The examples are clear and the troubleshooting section is particularly helpful.
UPGRADE_NOTES.md (1)
29-52: Well-structured version documentation.The version-specific upgrade information is clear, comprehensive, and actionable. The inclusion of migration principles and references to detailed documentation (
MIGRATIONS.md) is particularly helpful for users performing upgrades.resources/config/routes.yaml (1)
254-265: CSRF protection now applied to media upload routes.The new routes correctly apply the
auth-csrffilter, addressing the previous review concern about missing CSRF protection. However, theAuthCsrfFilterimplementation (reviewed separately) has a critical bug that must be fixed for these routes to function properly.Note: The past review comment mentioned
filter: auth csrf(space-separated), but the implementation usesfilter: auth-csrf(hyphenated), which is the correct filter name for the composite filter.src/Cms/Auth/Filters/AuthCsrfFilter.php (4)
1-26: LGTM!The class structure, documentation, and property declarations are well-designed. The exempt methods list (GET, HEAD, OPTIONS) correctly identifies safe HTTP methods that don't require CSRF protection.
27-44: LGTM!The constructor properly implements dependency injection with sensible defaults. The closure-based approach for the parent Filter constructor is appropriate.
58-77: LGTM!The CSRF validation logic correctly exempts safe HTTP methods and properly validates tokens for state-changing requests. Logging of security events is appropriate.
79-98: Post::filterScalar is an external API from the neuron-php/data dependency and cannot be verified within this repository.Line 85 uses
\Neuron\Data\Filters\Post::filterScalar('csrf_token')which is defined in the externalneuron-php/data(0.9.*) package, not in this codebase. While the API is used consistently across the codebase (also in CsrfFilter.php) and documented as applying "proper input sanitization," verification of its actual implementation requires inspection of the external package source code, which is outside the scope of this repository.src/Cms/Services/Content/EditorJsRenderer.php (2)
91-106: LGTM! Clean refactoring.The delegation to
renderListItemimproves code organization and sets up proper handling for nested list structures.
108-153: LGTM! Well-structured nested list handling.The method correctly handles both legacy string format and nested list objects. The recursive approach properly processes nested structures with appropriate indentation.
src/Cms/Repositories/DatabasePostRepository.php (10)
9-9: Namespace updated to match project refactoring.The import path change from
Neuron\Data\Setting\SettingManagertoNeuron\Data\Settings\SettingManageraligns with the broader namespace reorganization mentioned in the PR summary.
14-34: LGTM! Documentation accurately reflects hybrid approach.The class documentation and constructor comments clearly indicate the ORM-based approach while noting that raw SQL is retained for methods like
getByCategoryandgetByTagthat require JOINs.
39-110: LGTM! Good use of eager loading pattern.The new
loadRelationsmethod effectively prevents N+1 query issues by eagerly loading categories and tags. The use of raw SQL for JOINs is appropriate given the current ORM limitations noted in the code.
115-147: LGTM! Transaction properly wraps all operations.The use of
Post::transactionon line 124 ensures atomicity across post creation and relation synchronization, addressing the previous review concern about partial updates.
152-182: LGTM! Update operation properly transactional.The transaction wrapping ensures atomic updates to both the post and its relations.
187-193: LGTM! Clean ORM-based deletion.The deletion logic is straightforward and correctly returns a boolean. The comment about cascade deletes is helpful context.
198-323: LGTM! Query methods properly migrated to ORM.The migration to ORM-based queries is well-executed. The TODO comments for
getByCategoryandgetByTagappropriately note the need for future JOIN support in the ORM QueryBuilder.
325-339: LGTM! Race condition properly addressed.The atomic
incrementoperation on lines 334-336 correctly addresses the previous review concern about lost increments under concurrency. The documentation clearly explains the fix.
341-360: LGTM! Transaction concern addressed at caller level.While
syncCategoriesdoesn't have its own transaction, it's properly called withinPost::transactionblocks in bothcreate(line 135) andupdate(line 174) methods, addressing the previous review concern about atomicity.
362-381: LGTM! Consistent with syncCategories pattern.The
syncTagsmethod mirrors thesyncCategoriesimplementation and is similarly called within transaction boundaries increate(line 142) andupdate(line 178).src/Cms/Dtos/CreateUserDto.yaml (1)
1-23: LGTM! Documentation clarifies DTO design decision.The added header comment clearly explains why the flat structure is used instead of composition, referencing the RequestMapper limitation. This is helpful for future maintainers who might wonder about the DTO structure.
src/Cms/Dtos/CreateCategoryDto.yaml (1)
1-20: LGTM! Consistent documentation pattern.The header comment follows the same pattern as other DTOs in this PR, maintaining consistency across the codebase.
src/Cms/Dtos/CreatePostDto.yaml (1)
1-36: LGTM! Documentation maintains consistency.The header comment is consistent with the pattern established in other Create*Dto files.
src/Cms/Dtos/UserCredentialsDto.yaml (1)
1-34: LGTM! Well-documented reference DTO.The extensive documentation clearly explains the purpose, current limitations, and future usage path for this reusable DTO. The field definitions are appropriate for user credentials with reasonable validation constraints.
src/Cms/Dtos/CategoryFieldsDto.yaml (1)
1-27: LGTM! Reusable category fields definition.The DTO follows the same pattern as
UserCredentialsDto, providing a reusable field definition for category-related operations. Documentation is clear and field constraints are reasonable.src/Cms/Dtos/PostFieldsDto.yaml (1)
1-43: LGTM! Comprehensive post fields definition.The DTO captures all essential post fields with appropriate validation constraints. The documentation is consistent with other reference DTOs in this PR.
tests/Unit/Cms/Models/PostTest.php (5)
323-345: LGTM!Test correctly verifies extraction of plain text from a simple list with string items.
347-379: LGTM!Test correctly verifies extraction of plain text from nested lists, including both simple string items and object items with nested content.
381-418: LGTM!Test correctly verifies extraction from multi-level nested lists and includes the critical assertion that "Array" strings do not appear in the output, addressing the concern from previous reviews.
420-464: LGTM!Test provides excellent coverage of mixed content extraction, including headers, paragraphs, and nested lists in a single content structure.
466-504: LGTM!Test correctly verifies that HTML tags are stripped while preserving the text content, which is important for both security and plain-text rendering.
src/Cms/Models/Post.php (4)
21-22: LGTM!Sensible defaults for both properties. The empty Editor.js JSON structure ensures consistent behavior when no content is set.
599-599: LGTM!Addition of
content_rawto the array representation is consistent with the new property and necessary for serialization.
617-656: LGTM!The plain text extraction logic is well-implemented. The use of
extractListTextfor list blocks (line 639) properly addresses the nested list handling concern from previous reviews, preventing "Array" strings in the output.
668-697: LGTM! Past review concern resolved.This method correctly implements the nested list extraction logic suggested in previous reviews. It properly handles:
- Simple string items
- Object items with
contentproperty- Recursive extraction of nested
itemsarraysThe implementation aligns with the Editor.js List plugin v1.9+ nested structure and prevents "Array" strings in the output.
src/Cms/Events/MaintenanceModeEnabledEvent.php (1)
1-40: LGTM! Clean event implementation.The event class is well-structured and thoroughly documented. The implementation correctly follows the IEvent interface pattern and is consistent with the related MaintenanceModeDisabledEvent.
src/Cms/Maintenance/MaintenanceManager.php (1)
97-107: Good implementation: User captured before deletion.The logic correctly captures the disabling user before deleting the maintenance file, preventing loss of this information. The event is properly emitted only on successful file removal.
src/Cms/Services/Media/CloudinaryUploader.php (10)
27-31: LGTM!The constructor properly injects dependencies and initializes the Cloudinary instance. Exception handling is clearly documented.
39-57: LGTM!The initialization method properly validates required Cloudinary configuration and provides a clear error message directing users to the config file.
67-88: LGTM!The upload method properly validates file existence, handles errors gracefully, and provides clear error messages with context.
98-117: LGTM! SSRF protection implemented.The
uploadFromUrlmethod now includes comprehensive SSRF protection viavalidateUrlAgainstSsrf, addressing the security concern raised in the previous review. The implementation properly validates URLs before passing them to Cloudinary.
126-139: LGTM!The delete method correctly uses Cloudinary's destroy API and properly interprets the result status.
154-207: LGTM! Comprehensive SSRF protection.The URL validation method implements robust SSRF protection:
- HTTPS-only requirement
- URL format and parsing validation
- IP address validation for both direct IPs and resolved hostnames
- Blocking of private, loopback, link-local, and cloud metadata ranges
Note: DNS rebinding (TOCTOU) is a theoretical concern where DNS could change between validation and Cloudinary's actual request, but this is a known limitation of DNS-based SSRF protection and difficult to mitigate without controlling the HTTP client directly. The current implementation follows industry best practices.
215-246: LGTM!The DNS resolution method properly handles both IPv4 and IPv6 records and gracefully returns an empty array when resolution fails (which is handled appropriately by the caller).
263-271: LGTM!The IP validation method correctly uses PHP's built-in flags to detect private and reserved IP ranges for both IPv4 and IPv6.
279-305: LGTM!The options builder properly merges user-provided options with config defaults and uses sensible fallbacks.
313-325: LGTM!The result formatter properly normalizes Cloudinary's response into a consistent structure and correctly prefers
secure_url(HTTPS) overurl.tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php (15)
17-29: LGTM!The test setup uses in-memory settings, ensuring tests are isolated and don't depend on external configuration files. This is a best practice for unit testing.
31-41: LGTM!The test properly verifies that the constructor throws an exception when Cloudinary configuration is missing, ensuring proper validation at initialization.
43-51: LGTM!The test correctly verifies that the upload method validates file existence before attempting to upload.
53-61: LGTM!The test properly verifies that malformed URLs are rejected.
63-71: LGTM!The test properly verifies the HTTPS-only security requirement for URL uploads.
73-81: LGTM!The test verifies that non-HTTPS protocols like FTP are also rejected, ensuring comprehensive protocol validation.
83-95: LGTM!The test properly handles system-dependent DNS resolution behavior for localhost while ensuring the security goal (blocking localhost) is achieved in all cases. The flexible assertion pattern is well-documented.
97-105: LGTM!The test verifies that loopback addresses (127.0.0.1) are blocked, preventing SSRF attacks to localhost services.
107-115: LGTM!The test verifies that the private IP range 10.0.0.0/8 is blocked, preventing SSRF to internal networks.
117-125: LGTM!The test verifies that the private IP range 192.168.0.0/16 is blocked.
127-135: LGTM!The test verifies that the private IP range 172.16.0.0/12 is blocked, completing coverage of all RFC1918 private ranges.
137-146: LGTM!The test verifies that link-local addresses (169.254.0.0/16) are blocked. This range is also significant for cloud environments where it may be used for instance metadata services.
164-180: LGTM!The integration test stub is well-documented and provides clear guidance for enabling integration testing when needed. Marking it as incomplete is appropriate for the unit test suite.
182-195: LGTM!The integration test stub properly documents requirements for enabling real URL upload testing.
197-209: LGTM!The integration test stub for the delete operation is well-documented and appropriately marked as incomplete.
tests/Unit/Cms/ContentControllerTest.php (4)
19-40: LGTM! Clean refactoring from properties to local variables.The refactoring from class properties (
$this->root,$this->settings,$this->router) to local variables is appropriate since none of the test methods directly reference these values. This improves test clarity and reduces unnecessary instance state.
59-59: Good addition for test isolation.Adding
DtoFactoryServiceto the Registry cleanup ensures proper isolation between tests. This aligns with the cleanup patterns introduced elsewhere in the PR.
171-171: Appropriate annotation for session-related tests.Adding
@preserveGlobalState disabledto tests that use@runInSeparateProcessand interact withSessionManageris correct. This prevents global state pollution issues with PHP sessions.Also applies to: 194-194
6-6: Verify the namespace exists in the neuron-php/data dependency.The import statement references
Neuron\Data\Settings\Source\Memoryfrom the externalneuron-php/datapackage (0.9.*). This namespace change fromNeuron\Data\Setting\Source\Memory(singular) toNeuron\Data\Settings\Source\Memory(plural) appears across multiple test files. The actual class definition should be verified by checking the external package documentation or runningcomposer installto confirm the namespace is correct in the dependency version being used.
| // Get who is disabling before we delete the file | ||
| $disabledByUser = $disabledBy ?? get_current_user(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Searching for enable() method calls ==="
rg -n -C3 '->enable\(' --type=php
echo -e "\n=== Searching for disable() method calls ==="
rg -n -C3 '->disable\(' --type=php
echo -e "\n=== Searching for the MaintenanceManager class definition ==="
rg -n 'class MaintenanceManager' --type=php -A 5
echo -e "\n=== Searching for enable() and disable() method signatures ==="
rg -n 'public function (enable|disable)\(' --type=php -A 2Repository: Neuron-PHP/cms
Length of output: 2500
Ensure $disabledBy parameter is always passed by callers.
The fallback to get_current_user() will return the PHP process owner (e.g., "www-data"), not the authenticated user, providing no meaningful audit trail. Since the parameter is optional, document that callers must always pass the actual user identifier to maintain accurate audit records.
🤖 Prompt for AI Agents
In src/Cms/Maintenance/MaintenanceManager.php around lines 92-93, remove the
fallback to get_current_user() and make the $disabledBy parameter required
(remove the default/optional behavior) and update the method docblock to state
callers must provide the authenticated user identifier; then update every call
site to pass the actual user id (search the codebase for this method, adjust
callers and tests accordingly) and delete the line that assigns $disabledByUser
using get_current_user().
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/Unit/Cms/BlogControllerTest.php (2)
54-56: Test isolation issue with static ORM state.The
Model::setPdo()call sets static state that persists across tests. This has already been flagged in a previous review. Consider adding cleanup intearDown()to reset the PDO connection and prevent test interdependencies.
100-120: Missing test user data for referential integrity.The users table is created but no test users are inserted, while test posts reference
author_id(default = 1). This has already been flagged in a previous review. Consider inserting test user records to maintain referential integrity in the test environment.
🧹 Nitpick comments (1)
tests/Unit/Cms/BlogControllerTest.php (1)
290-360: Consider strengthening test assertions.Multiple test methods only assert that the result is a string without verifying the actual behavior or content. For more robust tests, consider adding assertions that:
- Verify the returned content contains expected post titles or data
- Check that filtering logic correctly includes/excludes posts
- Validate error handling for edge cases
This would improve test reliability and catch potential regressions more effectively.
Example: Strengthening assertions for testIndexReturnsPublishedPosts
public function testIndexReturnsPublishedPosts(): void { $this->createTestPost( 'Published Post 1', 'published-1', Post::STATUS_PUBLISHED ); $this->createTestPost( 'Published Post 2', 'published-2', Post::STATUS_PUBLISHED ); $this->createTestPost( 'Draft Post', 'draft-1', Post::STATUS_DRAFT ); $blog = $this->createBlogWithInjectedRepositories(); $result = $blog->index( new Request() ); $this->assertIsString( $result ); // Verify published posts are present $this->assertStringContainsString( 'Published Post 1', $result ); $this->assertStringContainsString( 'Published Post 2', $result ); // Verify draft is not present $this->assertStringNotContainsString( 'Draft Post', $result ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Cms/Events/MaintenanceModeDisabledEvent.php(1 hunks)tests/Unit/Cms/BlogControllerTest.php(4 hunks)
🔇 Additional comments (5)
src/Cms/Events/MaintenanceModeDisabledEvent.php (1)
1-38: LGTM! Previous issue has been resolved.The unused
Userimport flagged in the previous review has been removed. The current implementation is clean, well-documented, and correctly implements theIEventinterface. The use of constructor property promotion withreadonlymodifier is appropriate for an immutable event class.tests/Unit/Cms/BlogControllerTest.php (4)
16-16: LGTM!The
Modelimport is correctly added to support ORM initialization in the test setup.
72-73: LGTM!The path updates correctly reflect the new directory structure for the test environment.
129-129: LGTM!The
content_rawcolumn with default Editor.js JSON structure correctly supports the block-based editor upgrade mentioned in the PR objectives.
13-14: Namespace update toNeuron\Data\Settingsis correct and complete.The imports have been properly updated from the singular
Neuron\Data\Settingto the pluralNeuron\Data\Settingsnamespace. The codebase is consistent throughout, with all references already migrated.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/Cms/Cli/Commands/Install/UpgradeCommand.php (3)
27-27: Refactor nesteddirname()calls for maintainability.Five nested
dirname()calls are fragile and difficult to maintain. If the directory structure changes, this breaks silently. Consider using a more explicit path calculation or a constant.🔎 Proposed refactor
- $this->_componentPath = dirname( dirname( dirname( dirname( dirname( __DIR__ ) ) ) ) ); + // From src/Cms/Cli/Commands/Install/UpgradeCommand.php up to package root + $this->_componentPath = realpath( __DIR__ . '/../../../../../' );Alternatively, define the component path as a constant in a bootstrap or configuration file.
61-61: Unused parameter can be removed if not required by parent.The
$parametersparameter is unused in this method. If the parent classCommanddoesn't require this signature, consider removing it for clarity.🔎 Proposed refactor if allowed
- public function execute( array $parameters = [] ): int + public function execute(): intNote: Only apply this change if the parent class interface permits it.
435-448: Method namerunMigrations()is misleading.The method is named
runMigrations()but doesn't actually run migrations—it only displays instructions to the user. This creates a false expectation for anyone reading the code at line 151 where it's called.Consider renaming to
displayMigrationInstructions()orpromptMigrationRun()for clarity.🔎 Proposed refactor
- private function runMigrations(): bool + private function displayMigrationInstructions(): bool { - $this->output->writeln( "Running migrations...\n" ); - - // For now, instruct user to run migrations manually - // In future, could integrate with MigrationManager - $this->output->info( "Run: php neuron db:migrate" ); return true; }And update the call site at line 151:
- $this->runMigrations(); + $this->displayMigrationInstructions();
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Cms/Cli/Commands/Install/UpgradeCommand.php(1 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/Cms/Cli/Commands/Install/UpgradeCommand.php
61-61: Avoid unused parameters such as '$parameters'. (undefined)
(UnusedFormalParameter)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Cms/Cli/Commands/Install/UpgradeCommand.php (1)
394-403: Handle config file copy failures.The
copy()operation can fail, but failures are silently ignored. If a copy fails, the method continues and returnstrue, which is inconsistent withcopyNewMigrations()that properly handles failures.🔎 Proposed fix
foreach( $exampleFiles as $sourceFile ) { $fileName = basename( $sourceFile ); $destFile = $configDest . '/' . $fileName; - if( copy( $sourceFile, $destFile ) ) + if( !copy( $sourceFile, $destFile ) ) { - $this->output->writeln( " ✓ Updated: $fileName" ); + $this->output->warning( " ✗ Failed to copy: $fileName" ); + return false; } + $this->output->writeln( " ✓ Updated: $fileName" ); }
🧹 Nitpick comments (2)
src/Cms/Cli/Commands/Install/UpgradeCommand.php (2)
140-141: Remove duplicate comment.There are two consecutive "// Display summary" comments where only one is needed.
🔎 Proposed fix
- // Display summary // Display summary $this->displaySummary();
61-61: Static analysis hint: unused parameter.The
$parametersargument is part of the Command interface but not used in this implementation. This is acceptable as it maintains interface compliance, though you could document it with@param array $parameters Unused in this commandif desired.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Cms/Cli/Commands/Install/UpgradeCommand.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Cms/Cli/Commands/Install/UpgradeCommand.php (1)
src/Cms/Cli/Commands/Install/InstallCommand.php (2)
__construct(25-32)displaySummary(910-925)
🪛 PHPMD (2.15.0)
src/Cms/Cli/Commands/Install/UpgradeCommand.php
61-61: Avoid unused parameters such as '$parameters'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (4)
src/Cms/Cli/Commands/Install/UpgradeCommand.php (4)
164-216: LGTM!The manifest loading logic is well-implemented with proper error handling. The use of
json_last_error()correctly detects JSON parse failures, and the fallback for old installations without a manifest is a nice touch.
305-360: LGTM!The migration copy logic properly handles errors by checking the return value of
copy()and returning false on failure. The validation of source file existence and directory creation is also well done.
365-375: LGTM!The conservative approach of not automatically overwriting view files is prudent, as it preserves user customizations. Providing the package views location for manual comparison is helpful.
411-431: LGTM!The manifest update logic is clean and correct. The unused variable issue from the previous review has been addressed, and error handling for
file_put_contents()is properly implemented.
…ms into feature/blog-editor-upgrade
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/Cms/Cli/Commands/Install/UpgradeCommand.php (1)
394-410: Inconsistent error handling for configuration file copies.When
copy()fails (line 403), an error is displayed but the method still returnstrue(line 409). This is inconsistent withcopyNewMigrations()which returnsfalseon copy failure, and could allow the upgrade to appear successful despite missing configuration examples.🔎 Proposed fix
+ $copyFailed = false; + foreach( $exampleFiles as $sourceFile ) { $fileName = basename( $sourceFile ); $destFile = $configDest . '/' . $fileName; if( copy( $sourceFile, $destFile ) ) { $this->output->writeln( " ✓ Updated: $fileName" ); } else { $this->output->error( " ✗ Failed to copy $fileName from $sourceFile to $destFile" ); + $copyFailed = true; } } - return true; + return !$copyFailed;src/Cms/Models/Post.php (2)
500-534: Previous feedback addressed.The JSON encoding now properly checks for failures and throws
JsonExceptionwith descriptive error messages. This is consistent with thesetContentArraymethod behavior.
670-709: Previous feedback addressed - nested list extraction implemented.The
extractListTextmethod now correctly handles both simple string items and nested list structures withcontentanditemsproperties, matching the Editor.js List v1.9+ data structure.src/Cms/Repositories/DatabasePageRepository.php (1)
169-179: Previous feedback addressed - atomic increment implemented.The view count increment now uses an atomic ORM operation (
increment('view_count', 1)) which avoids the race condition from the fetch-increment-save pattern.
🧹 Nitpick comments (8)
src/Cms/Cli/Commands/Install/UpgradeCommand.php (4)
27-27: Consider simplifying the dirname chain.The nested
dirname()calls (5 levels) are difficult to read and maintain. Consider using a more explicit approach for clarity.🔎 Alternative approaches
- $this->_componentPath = dirname( dirname( dirname( dirname( dirname( __DIR__ ) ) ) ) ); + // Go up 5 levels: src/Cms/Cli/Commands/Install -> root + $this->_componentPath = dirname( __DIR__, 5 );Note: The second parameter to
dirname()was added in PHP 7.0.
61-61: Unused$parametersparameter.The
$parametersparameter is defined but never used in the method body. If this signature is required by the parentCommandclass, consider adding a docblock comment to clarify, or remove the parameter if it's not needed.
140-141: Remove duplicate comment.The comment "Display summary" appears twice consecutively.
🔎 Proposed fix
- // Display summary // Display summary $this->displaySummary();
440-450: Method name doesn't match implementation.The method
runMigrations()doesn't actually run migrations—it only displays an instruction message. Consider renaming todisplayMigrationInstructions()or similar to avoid confusion, or implement actual migration execution if that's the intent.src/Cms/Models/Post.php (2)
122-128: Consider addingJSON_THROW_ON_ERRORfor consistency.The
getContent()method usesjson_decodewith a fallback, but silently ignores decode errors. While the fallback is safe, consider usingJSON_THROW_ON_ERRORfor consistency withsetContentArray()which throws on encoding errors.🔎 Optional: Use try-catch for consistency
public function getContent(): array { - return json_decode( $this->_contentRaw, true ) ?? ['blocks' => []]; + try + { + $decoded = json_decode( $this->_contentRaw, true, 512, JSON_THROW_ON_ERROR ); + return $decoded ?? ['blocks' => []]; + } + catch( \JsonException $e ) + { + return ['blocks' => []]; + } }
647-657: Consider handling thechecklistblock type if using the standalone @editorjs/checklist plugin.The standalone Editor.js checklist plugin stores items with
textandcheckedproperties, which differ from the list block structure. If checklists are or will be extracted as a separate block type (type: "checklist"), they won't be processed by the current match statement.Note: If using Editor.js List 2.0, checklist is a style within the list block and is already handled by the 'list' case.
src/Cms/Repositories/DatabasePageRepository.php (1)
24-27: Consider removing unused$settingsparameter or adding validation.The constructor accepts
SettingManager $settingsbut doesn't use it. This could confuse callers. Either remove the parameter (breaking change) or add a comment explaining it's kept for interface compatibility.public function __construct( SettingManager $settings ) { - // No longer need PDO - ORM is initialized in Bootstrap + // Settings parameter kept for interface compatibility with IPageRepository + // ORM is initialized globally in Bootstrap }src/Cms/Repositories/IUserRepository.php (1)
58-65: Consider using a more explicit return type for "not found" case.Returning
-1for "user not found" is a magic number convention. Consider throwing an exception or using a nullable return type for clearer semantics.Alternative approach:
/** * @return int New failed login attempts count * @throws UserNotFoundException if user not found */ public function incrementFailedLoginAttempts( int $userId ): int;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Cms/Auth/Filters/AuthCsrfFilter.php(1 hunks)src/Cms/Cli/Commands/Install/UpgradeCommand.php(1 hunks)src/Cms/Models/Post.php(5 hunks)src/Cms/Repositories/DatabaseCategoryRepository.php(8 hunks)src/Cms/Repositories/DatabaseEmailVerificationTokenRepository.php(2 hunks)src/Cms/Repositories/DatabasePageRepository.php(5 hunks)src/Cms/Repositories/DatabasePasswordResetTokenRepository.php(2 hunks)src/Cms/Repositories/DatabasePostRepository.php(13 hunks)src/Cms/Repositories/DatabaseTagRepository.php(7 hunks)src/Cms/Repositories/DatabaseUserRepository.php(5 hunks)src/Cms/Repositories/IUserRepository.php(1 hunks)src/Cms/Services/Auth/Authentication.php(4 hunks)tests/Unit/Cms/Services/AuthenticationTest.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Cms/Repositories/DatabasePasswordResetTokenRepository.php
🧰 Additional context used
🧬 Code graph analysis (8)
src/Cms/Services/Auth/Authentication.php (7)
src/Cms/Events/UserLoginFailedEvent.php (1)
UserLoginFailedEvent(23-44)src/Cms/Models/User.php (2)
isLockedOut(332-340)isActive(187-190)src/Cms/Repositories/DatabaseUserRepository.php (4)
incrementFailedLoginAttempts(154-169)setLockedUntil(201-211)resetFailedLoginAttempts(179-190)findById(37-40)src/Cms/Repositories/IUserRepository.php (4)
incrementFailedLoginAttempts(65-65)setLockedUntil(82-82)resetFailedLoginAttempts(73-73)findById(17-17)src/Cms/Auth/SessionManager.php (1)
set(99-103)src/Cms/Events/UserLoginEvent.php (1)
UserLoginEvent(24-43)src/Cms/Events/UserLogoutEvent.php (1)
UserLogoutEvent(24-41)
src/Cms/Repositories/DatabaseCategoryRepository.php (1)
src/Cms/Models/Category.php (3)
toArray(182-192)setId(45-49)getId(37-40)
src/Cms/Repositories/IUserRepository.php (2)
src/Cms/Repositories/DatabaseUserRepository.php (3)
incrementFailedLoginAttempts(154-169)resetFailedLoginAttempts(179-190)setLockedUntil(201-211)src/Cms/Models/User.php (3)
incrementFailedLoginAttempts(296-300)resetFailedLoginAttempts(305-310)setLockedUntil(323-327)
src/Cms/Repositories/DatabaseTagRepository.php (2)
src/Cms/Repositories/DatabaseUserRepository.php (3)
create(69-90)__sleep(216-220)__wakeup(225-228)src/Cms/Models/Tag.php (3)
toArray(163-172)setId(44-48)getId(36-39)
tests/Unit/Cms/Services/AuthenticationTest.php (6)
src/Cms/Repositories/DatabaseCategoryRepository.php (2)
__sleep(184-188)__wakeup(193-196)src/Cms/Repositories/DatabaseEmailVerificationTokenRepository.php (2)
__sleep(139-143)__wakeup(148-151)src/Cms/Repositories/DatabasePasswordResetTokenRepository.php (2)
__sleep(121-125)__wakeup(130-133)src/Cms/Repositories/DatabasePostRepository.php (2)
__sleep(448-452)__wakeup(457-460)src/Cms/Repositories/DatabaseTagRepository.php (2)
__sleep(168-172)__wakeup(177-180)src/Cms/Repositories/DatabaseUserRepository.php (2)
__sleep(216-220)__wakeup(225-228)
src/Cms/Repositories/DatabasePostRepository.php (5)
src/Cms/Models/Post.php (5)
fromArray(488-597)getId(57-60)toArray(604-621)setId(65-69)incrementViewCount(321-325)src/Cms/Repositories/DatabaseCategoryRepository.php (4)
findBySlug(45-48)create(77-98)count(150-153)all(142-145)src/Cms/Repositories/DatabasePageRepository.php (5)
findBySlug(41-45)create(50-65)count(154-164)all(101-118)incrementViewCount(171-179)src/Cms/Repositories/DatabaseTagRepository.php (4)
findBySlug(45-48)create(61-82)count(134-137)all(126-129)src/Cms/Repositories/DatabaseUserRepository.php (3)
create(69-90)count(141-144)all(133-136)
src/Cms/Repositories/DatabaseEmailVerificationTokenRepository.php (6)
src/Cms/Repositories/DatabaseCategoryRepository.php (2)
__sleep(184-188)__wakeup(193-196)src/Cms/Repositories/DatabasePasswordResetTokenRepository.php (2)
__sleep(121-125)__wakeup(130-133)src/Cms/Repositories/DatabasePostRepository.php (2)
__sleep(448-452)__wakeup(457-460)src/Cms/Repositories/DatabaseTagRepository.php (2)
__sleep(168-172)__wakeup(177-180)src/Cms/Repositories/DatabaseUserRepository.php (2)
__sleep(216-220)__wakeup(225-228)tests/Unit/Cms/Services/AuthenticationTest.php (2)
__sleep(28-32)__wakeup(34-37)
src/Cms/Repositories/DatabaseUserRepository.php (1)
src/Cms/Models/User.php (7)
toArray(413-433)setId(69-73)getId(61-64)incrementFailedLoginAttempts(296-300)getFailedLoginAttempts(279-282)resetFailedLoginAttempts(305-310)setLockedUntil(323-327)
🪛 PHPMD (2.15.0)
src/Cms/Cli/Commands/Install/UpgradeCommand.php
61-61: Avoid unused parameters such as '$parameters'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (57)
src/Cms/Repositories/DatabaseEmailVerificationTokenRepository.php (2)
7-7: LGTM! Namespace updated correctly.The import path has been updated from
Neuron\Data\Setting\SettingManagertoNeuron\Data\Settings\SettingManager, which aligns with the project-wide namespace refactoring mentioned in the PR summary.
136-151: LGTM! Serialization pattern is consistent across repositories.The
__sleep()and__wakeup()methods correctly handle PHPUnit process isolation by preventing PDO serialization. This pattern matches the implementation in other repository classes (DatabaseCategoryRepository, DatabasePasswordResetTokenRepository, DatabasePostRepository, etc.).Note that after unserialization, the
$_pdoproperty will be null until re-initialized by test setup, which means the object temporarily cannot be used until external re-initialization occurs.src/Cms/Cli/Commands/Install/UpgradeCommand.php (6)
175-196: Proper JSON error handling implemented.The JSON parsing correctly uses
json_last_error()to detect parse failures and includes detailed error messages viajson_last_error_msg(). This handles edge cases where valid JSON (likenullor{}) might otherwise be misinterpreted as parse errors.
415-435: LGTM: Manifest update with proper error handling.The method correctly updates the manifest with package migrations and version info, properly checks the
file_put_contents()return value, and returns a boolean result that's now checked by the caller (lines 135-138).
135-138: Proper handling of manifest update failure.The return value of
updateInstalledManifest()is now checked, and a warning is displayed if it fails. The approach of warning rather than failing the entire upgrade is reasonable since the actual file operations succeeded.
305-360: Excellent error handling in migration copy.The method demonstrates proper error handling patterns:
- Validates source files exist before copying
- Checks
copy()return values- Returns
falseimmediately on failure- Provides clear success/failure feedback
This is a good reference pattern for file operations in this command.
221-289: Helper methods are well-structured.The installation check, version display, and update detection logic are clear and correctly implemented:
isInstalled()validates multiple indicatorsdisplayVersionInfo()provides clear version comparisoncheckForUpdates()properly detects both migration and version changesgetNewMigrations()correctly usesarray_diffto find new migrations
365-375: Conservative view update approach is appropriate.The decision to require manual view updates rather than automatic copying is sensible, as it preserves user customizations. The informational output guides users on where to find package views if needed.
src/Cms/Models/Post.php (2)
21-22: LGTM!The new properties
_bodyand_contentRaware correctly initialized with sensible defaults. The empty JSON structure{"blocks":[]}is a proper default for Editor.js content.
156-169: LGTM!The
setContentArraymethod correctly handles JSON encoding failures by checking forfalseand throwing aJsonExceptionwith a descriptive message. This addresses the previous review feedback about error handling.src/Cms/Repositories/DatabaseTagRepository.php (4)
164-180: LGTM!The
__sleepand__wakeupmethods correctly handle PHPUnit process isolation, matching the pattern used in other repositories (as shown in the relevant code snippets fromDatabaseUserRepository,DatabaseCategoryRepository, etc.).
108-110: Verifysave()returns boolean.The ORM's
save()method is expected to returnbooland this expectation is accurate for standard Laravel Eloquent implementations. Ensure the model class being saved confirms to this contract.
75-82: Verify that the Tag model uses auto-incrementing primary key.The code correctly assumes
Tag::create()returns a model with a populated ID. Standard Laravel Eloquent behavior hydrates the auto-generated ID after insertion. However, verify that the tags table schema usesauto_incrementon the ID column; if defined as a non-auto-incrementing primary key, the returned model may haveidset to0.
18-32: Verify if Neuron ORM provides method to access PDO connection from Model class.The observation is accurate:
$_pdois used only inallWithPostCount(), andDatabasePageRepositorydemonstrates the ORM works without storing PDO. However, the suggestion to useModel::getPdo()or similar requires confirming that Neuron-PHP ORM exposes a method to retrieve the globally-set PDO connection for raw SQL queries.src/Cms/Repositories/DatabasePageRepository.php (3)
34-35: LGTM!Good use of eager loading with
Page::with('author')to avoid N+1 queries when accessing author data.Also applies to: 43-44
101-118: Bothget()andall()are valid ORM methods. The codebase correctly usesquery()->get()for methods with conditional parameters (status filter, limit, offset) and staticModel::orderBy()->all()for simple cases without parameters. DatabasePageRepository follows the same pattern as DatabasePostRepository. No changes needed.
58-64: toArray() includes all ORM-managed fields—no verification needed.The
Page::toArray()method explicitly includes bothcreated_atandupdated_attimestamps along with all other fields. These are serialized to strings and can be properly deserialized by the ORM's hydrate mechanism. No data loss occurs.Likely an incorrect or invalid review comment.
tests/Unit/Cms/Services/AuthenticationTest.php (2)
61-62: LGTM!Correctly initializes the ORM with the PDO connection before running tests. This ensures the ORM-based repository methods work correctly.
91-91: LGTM!The
two_factor_recovery_codescolumn addition aligns with the broader 2FA feature support in the CMS.src/Cms/Repositories/IUserRepository.php (1)
67-82: LGTM!The
resetFailedLoginAttemptsandsetLockedUntilmethods are well-documented and provide atomic operations for account security management. The implementations inDatabaseUserRepository(shown in relevant snippets) correctly use atomic updates to avoid race conditions.src/Cms/Services/Auth/Authentication.php (7)
49-56: LGTM! Event emission on user not found.The event emission logic is correct, and the timing attack mitigation via dummy hash (line 48) is appropriately placed before the event.
64-70: LGTM! Event emission on locked account.The event emission for locked accounts is correctly implemented with appropriate reason code.
77-83: LGTM! Event emission on inactive account.Consistent with other failure events and correctly implemented.
90-107: LGTM! Atomic handling of failed login attempts.The atomic increment and lock operations correctly prevent race conditions during concurrent login attempts. Event emission is appropriate.
111-115: LGTM! Atomic reset with user refresh.The atomic reset followed by refreshing the user from the database ensures the in-memory User object reflects the updated state. Good pattern.
145-145: LGTM! Login time tracking and event emission.The login time tracking enables session duration calculation at logout, and the event emission provides appropriate observability.
Also applies to: 152-158
166-203: LGTM! Logout with session duration and event emission.The logout flow correctly calculates session duration, handles missing login_time gracefully, and only emits the event when a user was actually logged in.
src/Cms/Auth/Filters/AuthCsrfFilter.php (4)
49-56: LGTM! Authentication check is correct.The authentication check correctly uses the
check()method from the Authentication service. The previous issue withisAuthenticated()has been properly addressed.
58-76: LGTM! CSRF validation logic is sound.The CSRF validation correctly exempts idempotent HTTP methods and properly validates tokens for state-changing requests.
82-98: LGTM! Token retrieval supports both form and AJAX requests.The token retrieval correctly checks both POST data (with filtering) and HTTP headers, providing good support for different submission methods.
103-118: LGTM! Helper methods are correctly implemented.The redirect and forbidden response helpers are properly implemented with appropriate security measures (htmlspecialchars) and execution control (exit).
src/Cms/Repositories/DatabaseCategoryRepository.php (8)
7-7: LGTM! Import path updated and PDO retention explained.The import path change aligns with the broader ORM migration, and the PDO retention is appropriately documented for the raw SQL queries that remain.
Also applies to: 30-31
39-71: LGTM! ORM-based finder methods.The ORM-based finder methods use standard patterns and are correctly implemented.
91-97: LGTM! ORM-based create with ID sync.The create method correctly uses ORM and syncs the generated ID back to the input object, consistent with other repositories in the PR.
124-125: LGTM! ORM-based update.The update method correctly delegates to the ORM's save operation.
134-136: LGTM! ORM-based delete.The delete method correctly uses ORM and returns an appropriate boolean result.
144-144: LGTM! ORM-based all and count methods.Both methods use standard ORM patterns and are correctly implemented.
Also applies to: 152-152
160-179: LGTM! Raw SQL with documented TODO for ORM migration.The continued use of raw SQL for JOIN and aggregation is appropriately documented with a TODO, as the ORM doesn't yet support these operations. The query structure is correct.
181-196: LGTM! Serialization helpers for PHPUnit.The serialization helpers correctly handle PDO connection lifecycle during test process isolation, consistent with other repositories in the PR.
src/Cms/Repositories/DatabasePostRepository.php (8)
41-74: LGTM! Post retrieval with eager relation loading.The findById and findBySlug methods correctly retrieve posts and eagerly load their relations using the new loadRelations helper.
79-110: LGTM! Relation loading helper.The loadRelations helper correctly loads categories and tags using JOIN queries and cleanly maps the results to model objects.
123-146: LGTM! Transactional create with relation syncing.The create method correctly wraps post creation and relation syncing in a transaction, ensuring atomicity. This addresses the past review concern about partial failures.
166-181: LGTM! Transactional update with relation syncing.The update method correctly uses a transaction to ensure atomic updates of the post and its relations, consistent with the create method and addressing past review concerns.
326-339: LGTM! Atomic view count increment.The atomic increment implementation correctly addresses the race condition mentioned in the past review. The documentation clearly explains the approach.
344-381: LGTM! Relation sync helpers.The sync helpers for categories and tags are correctly implemented. They are invoked within transactions in the create and update methods, ensuring atomicity and addressing the past review concern.
200-284: LGTM! Query methods with appropriate ORM usage.The query methods correctly use ORM where applicable and fall back to raw SQL for JOIN operations with appropriate TODO comments indicating future migration paths.
446-460: LGTM! Serialization helpers for PHPUnit.The serialization helpers are consistent with other repositories and correctly handle PDO connection lifecycle during test process isolation.
src/Cms/Repositories/DatabaseUserRepository.php (10)
7-7: LGTM! Import path updated and PDO made nullable.The import path change and nullable PDO property align with the ORM migration. The backwards compatibility note is clear.
Also applies to: 20-20, 30-31
39-63: LGTM! ORM-based finder methods.The finder methods correctly use ORM patterns consistent with other repositories in the PR.
83-89: LGTM! ORM-based create with ID sync.The create method follows the same pattern as other repositories, correctly syncing the generated ID back to the input object.
116-117: LGTM! ORM-based update.The update method correctly delegates to the ORM's save operation.
125-127: LGTM! ORM-based delete.The delete method correctly uses ORM and returns an appropriate boolean result.
135-135: LGTM! ORM-based all and count methods.Both methods use standard ORM patterns and are correctly implemented.
Also applies to: 143-144
146-169: LGTM! Atomic failed login attempts increment.The atomic increment correctly prevents race conditions during concurrent login attempts. Fetching the updated count after increment is appropriate.
171-190: LGTM! Atomic failed login attempts reset.The atomic reset correctly updates both failed_login_attempts and locked_until in a single operation, preventing race conditions.
192-211: LGTM! Atomic account lockout setter.The atomic update for locked_until correctly prevents race conditions and properly handles DateTime formatting.
213-228: LGTM! Serialization helpers for PHPUnit.The serialization helpers are consistent with other repositories and correctly handle PDO connection lifecycle during test process isolation.
| public function __sleep(): array | ||
| { | ||
| // Don't serialize PDO for process isolation | ||
| return ['_authentication', '_userRepository', '_sessionManager', '_passwordHasher']; | ||
| } | ||
|
|
||
| public function __wakeup(): void | ||
| { | ||
| // PDO will be re-initialized in setUp() | ||
| } |
There was a problem hiding this comment.
Serialization includes non-serializable objects.
The __sleep method returns fields that likely contain mocks (_sessionManager, _passwordHasher) and repository instances. These may not serialize properly. Consider returning an empty array like the repository classes do.
🔎 Suggested fix
public function __sleep(): array
{
// Don't serialize PDO for process isolation
- return ['_authentication', '_userRepository', '_sessionManager', '_passwordHasher'];
+ return [];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function __sleep(): array | |
| { | |
| // Don't serialize PDO for process isolation | |
| return ['_authentication', '_userRepository', '_sessionManager', '_passwordHasher']; | |
| } | |
| public function __wakeup(): void | |
| { | |
| // PDO will be re-initialized in setUp() | |
| } | |
| public function __sleep(): array | |
| { | |
| // Don't serialize PDO for process isolation | |
| return []; | |
| } | |
| public function __wakeup(): void | |
| { | |
| // PDO will be re-initialized in setUp() | |
| } |
🤖 Prompt for AI Agents
In tests/Unit/Cms/Services/AuthenticationTest.php around lines 28 to 37, the
__sleep method lists properties (_authentication, _userRepository,
_sessionManager, _passwordHasher) that may contain mocks or other
non-serializable objects; change __sleep to return an empty array so nothing is
serialized, leaving __wakeup as-is (PDO and other dependencies should be
re-initialized in setUp()), and run/update tests to ensure setup re-initializes
the needed properties.
Upgrades the blog editor to be a block based editor.
A ton of refactoring.
Summary by CodeRabbit
New Features
Database / Migrations
Security
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.